Added a follow-up commit to fully cover INSERT-time slug races for NOT NULL + pre-insert slug strategies (used by organizations).

Commit: 14bd675

What changed:

• Added around_create :retry_create_on_slug_unique_violation
• Retries only slug-related ActiveRecord::RecordNotUnique for persisted-slug models
• Recomputes slug before retry (create callback retries do not re-run validations)
• Added regression fixture model (StrictSlugModel) with slug null: false
• Added new regression suite: test/slugifiable/insert_race_retry_test.rb
  • proves retry success on INSERT-time collision
  • proves non-slug unique violations still bubble
  • proves retry limit enforcement
  • validates uniqueness behavior for strict NOT NULL slug models

Verification:

• slugifiable full suite: 263 runs, 522 assertions, 0 failures, 0 errors (6 skips)
• organizations suite (with local slugifiable path): 28 runs, 96 assertions, 0 failures, 0 errors

PR Review: Fix race condition in slug generation with retry mechanism

The core problem being solved is real and the overall approach is sound. Two hooks (around_create for INSERT-time collisions and after_create retry for post-insert slug assignment) give good coverage of different race windows. That said, there are a few correctness issues and code quality concerns worth addressing.

Correctness Issues

1. Redundant guard in retry_create_on_slug_unique_violation

self.slug = compute_slug if has_slug_method?

At this point, slug_persisted? has already been checked (it raises otherwise), and slug_persisted? is defined as has_slug_method? && self.attributes.include?("slug"). So has_slug_method? is always true here. The guard is dead code and suggests a misunderstanding of the control flow.

2. respond_to?(:id_changed?) guard is unnecessary

self.slug = compute_slug if slug.blank? || (respond_to?(:id_changed?) && id_changed?)

id_changed? is a standard ActiveRecord dirty tracking method available on every AR model. The respond_to? guard adds noise without defensive value.

3. around_create overhead for all models

around_create :retry_create_on_slug_unique_violation is registered for every model that includes Slugifiable::Model, including TestModelWithoutSlug and any user model without a slug column. The guard (raise unless slug_persisted?) lives inside the rescue block, meaning the callback wrapper runs unconditionally on every create call, but only does useful work in the error path for models with slug columns. This imposes a small but unnecessary overhead on all models.

Consider an early return at the start:

def retry_create_on_slug_unique_violation
  return yield unless slug_persisted?
  # ... retry logic
end

4. Duplicated retry loop

set_slug_with_retry and retry_create_on_slug_unique_violation both implement the same attempts = 0 / begin / rescue / attempts += 1 / raise if attempts > MAX / retry structure. A private helper would avoid the duplication and reduce the surface for drift if MAX_SLUG_GENERATION_ATTEMPTS semantics ever change.

5. slug_unique_violation? false-positive risk

def slug_unique_violation?(error)
  error.message.to_s.downcase.include?("slug")
end

Any table or column whose name contains "slug" (e.g., slug_version, reslugged_items) would match. This is a known trade-off but should be documented in a comment — right now it's easy to read this as foolproof. Also worth noting: the PostgreSQL error format includes the constraint name (e.g., "index_users_on_slug"), which still contains "slug", so it works, but a comment acknowledging the DB adapter differences would help maintainers.

6. Off-by-one in attempt counting

Both retry methods use raise if attempts > MAX_SLUG_GENERATION_ATTEMPTS. Since attempts starts at 0, this allows up to MAX_SLUG_GENERATION_ATTEMPTS + 1 total calls (initial + 10 retries = 11 total for the default of 10). The insert test correctly asserts MAX_SLUG_GENERATION_ATTEMPTS + 1, so the tests are accurate, but the variable name MAX_SLUG_GENERATION_ATTEMPTS implies a maximum of 10, not 11. Consider raise if attempts >= MAX_SLUG_GENERATION_ATTEMPTS to make the count match the constant name.

Test Concerns

test_retry_clears_slug_before_regeneration doesn't test clearing

def test_retry_clears_slug_before_regeneration
  model = TestModel.new(title: "Clear Slug Test")
  model.define_singleton_method(:compute_slug) { ... }
  model.send(:set_slug_with_retry)
  # asserts first slug is based on title

No collision is simulated here. The slug is never cleared and compute_slug is only called once. The test name promises one behavior but exercises a different one.

test_without_retry_concurrent_creates_would_fail is misleading

The comment says "without retry, these would fail" but the test runs with the fix in place. It's a valid regression test, but renaming it (e.g., test_many_sequential_creates_with_same_title_all_succeed) and updating the comment would remove the ambiguity.

test_max_retries_exceeded_raises_error is fragile

Assigning model.id = 999 without persisting and then stubbing save and compute_slug via define_singleton_method produces a non-representative state. The TestModel isn't actually in the database, so slug_persisted? behavior may differ from production. For the StrictSlugModel path (insert_race_retry_test.rb), the test_retry_limit_is_enforced does a better job of testing this with real DB interaction.

Significant test overlap with concurrency_test.rb

race_condition_retry_test.rb#test_sequential_creates_with_same_title_all_get_unique_slugs (20 records) and test_without_retry_concurrent_creates_would_fail (51 records) overlap substantially with concurrency_test.rb#test_rapid_sequential_creates_with_same_title (20 records) and test_many_records_same_title (50 records). The new tests add value only if they test something the existing suite does not — the retry mechanism in isolation. The high-volume integration scenarios are already covered.

Missing test: DB-level race via test_models table

The test_models table has no unique index on slug (only the Rails validation). This means set_slug_with_retry's rescue branch can never be triggered by a real DB constraint in tests — it can only be exercised through mocking. Adding a unique index to the test schema or an integration test with strict_slug_models-style setup for the after_create path would close this gap.

Minor Nits

• model.rb doesn't have frozen_string_literal: true (pre-existing), but the new methods return strings that would benefit from it.
• StrictSlugModel includes both before_validation :ensure_slug_for_insert and after_create :set_slug. After a successful create, set_slug_with_retry still calls self.save even when the slug is already correct (slug not blank, id not changed). This is a pre-existing redundant UPDATE per create — not introduced by this PR but worth noting as a follow-up.

Summary

The fix is on the right track. The around_create overhead issue is probably the most impactful to address before merging since it affects all users of the gem, not just the race-condition edge case.

🤖 Generated with Claude Code

Addressed Review Feedback

Pushed commit 25ac05e with the following fixes:

Code Quality

Correctness

Tests

• Updated test_retry_limit_is_enforced to expect MAX_SLUG_GENERATION_ATTEMPTS (was + 1)
• Removed unused variable warning

All 263 tests passing ✅

Code Review

Good problem statement — the race condition between concurrent slug generation is a real issue. The core retry concept is sound, but there are a few issues worth addressing before merging.

Critical: PostgreSQL transaction abort in retry_create_on_slug_unique_violation

The around_create retry will silently fail on PostgreSQL. When a RecordNotUnique occurs inside an active (non-savepoint) PostgreSQL transaction, PostgreSQL marks the entire transaction as aborted. All subsequent commands — including the retry INSERT — are rejected with:

ERROR: current transaction is aborted, commands ignored until end of transaction block

create! opens a real transaction, and around_create's yield runs inside it. When the INSERT fails and you rescue+retry, you're still inside that poisoned transaction. The retry INSERT will fail, too.

This is a well-known PostgreSQL gotcha. The standard fix is to wrap each attempt in a nested transaction (savepoint) so that a failed attempt only rolls back to the savepoint, leaving the outer transaction healthy:

def retry_create_on_slug_unique_violation
  return yield unless slug_persisted?
  with_slug_retry(-> { self.slug = compute_slug }) do
    self.class.transaction(requires_new: true) { yield }
  end
end

Note: set_slug_with_retry (used in after_create) does not share this problem. self.save inside an after_create callback opens a nested transaction, which Rails maps to a SAVEPOINT in PostgreSQL. A failed UPDATE rolls back only the savepoint, so retrying works correctly. The issue is specific to the around_create path.

The test suite uses SQLite in-memory, which does not abort transactions on constraint violations the way PostgreSQL does — so this bug won't surface in CI.

Significant: Double-save for StrictSlugModel

For null: false slug columns, after a successful around_create retry, after_create :set_slug still fires. At that point id_changed? is true (the ID was just assigned during INSERT), so set_slug_with_retry computes and saves a new slug via UPDATE, overwriting the one that was carefully assigned during the retry.

Concretely:

1. before_validation :ensure_slug_for_insert → slug = "acme"
2. Collision! around_create retry → slug = "acme-382910" → INSERT succeeds
3. after_create :set_slug → id_changed? is true → compute_slug → slug = "acme-499177" → UPDATE

The slug changes twice; the around_create retry work is thrown away, and there's an extra unnecessary UPDATE on every create for this model type.

Significant: Exception propagation can incorrectly retry the entire CREATE

after_create :set_slug → self.save (UPDATE) runs inside the around_create yield. If set_slug_with_retry exhausts its retries and raises, that RecordNotUnique propagates up through the yield and is caught by retry_create_on_slug_unique_violation. This triggers a retry of the entire yield — meaning a second INSERT for a record that's already in the database. The result depends on the adapter but is at best confusing.

The two retry layers (around_create + after_create) operating on the same exception path need a clear boundary (e.g., a sentinel flag or a marker exception subclass) so one doesn't shadow the other.

Minor: README still warns against null: false

The README has:

Your slug attribute SHOULD NOT have null: false in the migration...

This PR's intent is to make null: false columns work, but the README wasn't updated. Users following the README will still avoid null: false, making the new around_create retry path unreachable for them. The note should be updated or removed if null: false is now a supported use case.

Minor: Inconsistency in retry-exhaustion behavior

generate_unique_slug falls back to a timestamp-based suffix after MAX_SLUG_GENERATION_ATTEMPTS so it never fails (timestamp + random guarantees uniqueness). But with_slug_retry raises after MAX_SLUG_GENERATION_ATTEMPTS. These two paths share the same constant but have different contracts on exhaustion. Worth harmonising — or at least documenting why they intentionally differ.

Minor: No CHANGELOG entry

The existing pattern in CHANGELOG.md documents changes per release. This fix is notable enough (new callbacks, behavioural change for null: false columns) to warrant an entry.

What's good

• The PR description clearly explains the race condition and the solution approach.
• slug_unique_violation? is honest about its substring-matching trade-offs in the comment — that's the right call for cross-adapter support.
• with_slug_retry is clean shared logic, and the lambda pre_retry_action pattern correctly distinguishes the two callers' needs (nil vs recompute).
• StrictSlugModel as a test fixture is a thoughtful addition.
• test_insert_time_non_slug_record_not_unique_bubbles and test_retry_limit_is_enforced directly validate the guard rails.

The PostgreSQL transaction issue and the double-save interaction are the ones I'd want resolved before merge. Happy to discuss approaches if useful.

Addressed the latest review feedback in commit a41dc9b.

What was fixed

• PostgreSQL transaction-abort risk in around_create retries

  • Each create attempt now runs inside transaction(requires_new: true) savepoints in retry_create_on_slug_unique_violation.
  • This prevents poisoned outer transactions after RecordNotUnique on PostgreSQL.

• Double-save / slug overwrite for null: false pre-insert strategies

  • set_slug_with_retry now exits early unless slug.blank?, so after_create no longer recomputes/saves a second slug when one already exists.

• Retry boundary between create-path and after_create-path

  • with_slug_retry now supports a retry_if gate.
  • around_create retries only when !persisted?, so a post-insert after_create unique violation no longer triggers a second INSERT retry loop.

• Retry-exhaustion behavior clarification

  • Added a code comment documenting why with_slug_retry raises on exhaustion while generate_unique_slug has timestamp fallback behavior.

• Test coverage improvements requested in review

  • Added real DB unique index to test_models.slug so update-path retries are exercised against DB constraints.
  • Reworked fragile/misleading update-race tests to deterministic DB-backed race simulations.
  • Added insert-race tests for:
    • requires_new transaction wrapping
    • no redundant post-create recompute when slug already set
    • no accidental create-path retry when after_create retry exhausts

• Docs / changelog

  • Updated README null: false guidance to document supported pre-insert slug strategy.
  • Added Unreleased changelog entry for retry/race-condition behavior changes.

Verification

• bundle exec rake test passing:
  • 265 runs, 428 assertions, 0 failures, 0 errors, 6 skips

PR Review: Race condition retry mechanism

Overall this is a solid fix for a real problem. The two-path approach (around_create for NOT NULL slugs, set_slug_with_retry for nullable slugs) is architecturally correct. Tests are comprehensive. A few issues worth discussing before merge:

Bugs / Correctness

1. set_slug_with_retry always resets slug to nil before retry — won't help for ID-based strategies

def set_slug_with_retry
  return unless slug.blank?
  with_slug_retry(-> { self.slug = nil }) do
    self.slug = compute_slug
    self.save
  end
end

For generate_slug_based_on :id (or :id => :number), compute_slug_as_string hashes the record's ID, which is fixed post-insert. Every retry will produce the exact same slug and fail again until hitting the limit. The retry loop will exhaust all 10 attempts and raise rather than finding a unique slug. This matters less in practice (ID-based slugs collide only if two records share the same ID, which is impossible), but the logic is silently wrong for that case — and it could mislead future maintainers or cause confusion if someone derives a slug from another fixed attribute.

2. Off-by-one: MAX_SLUG_GENERATION_ATTEMPTS attempts are made, not N retries after the first

attempts += 1
raise if attempts >= MAX_SLUG_GENERATION_ATTEMPTS

The initial call plus 9 retries = 10 total attempts. So MAX_SLUG_GENERATION_ATTEMPTS is the total attempt count, not the retry count. The constant name ("max attempts") is accurate, but the comment on with_slug_retry says "retries up to MAX_SLUG_GENERATION_ATTEMPTS times" which is off-by-one. The tests verify the behavior correctly (10 total attempts), so this is a documentation issue rather than a runtime bug, but worth fixing the comment.

Design / Architecture

3. slug_unique_violation? string matching — the acknowledged false-positive risk is real and asymmetric

def slug_unique_violation?(error)
  error.message.to_s.downcase.include?("slug")
end

The comment correctly identifies the limitation, but it's worth naming the direction of harm. A false positive (erroneously treating a non-slug violation as a slug collision) silently swallows a real data integrity error, retries up to 10 times, and then re-raises. Any model with a column like old_slug, slug_history, or table reslugged_items that has a unique constraint would have its violations silently retried and then raised without explanation.

An alternative that would be more precise: check error.cause (the underlying database error) for the constraint name against known slug index names:

def slug_unique_violation?(error)
  # Check ActiveRecord's parsed cause first when available
  cause_message = error.cause&.message.to_s.downcase
  message = error.message.to_s.downcase
  [cause_message, message].any? { |m| m.match?(/\bslug\b/) }
end

Using word-boundary matching (\bslug\b) would at minimum avoid matching reslugged or slug_history. This doesn't fully solve the problem but reduces the false-positive surface.

4. around_create :retry_create_on_slug_unique_violation fires for every model, including those without slug columns

def retry_create_on_slug_unique_violation
  return yield unless slug_persisted?
  ...
end

The early return is correct, but this adds a method dispatch and slug_persisted? check to every create call on every model that includes Slugifiable::Model, even TestModelWithoutSlug. For a concern that may be included across many models, this is worth noting. Not a blocker, but could the callback be registered conditionally?

included do
  if columns_hash.key?("slug")
    around_create :retry_create_on_slug_unique_violation
  end
  # ...
end

This has its own complexity (schema must be loaded at include time) so the current approach may be the right tradeoff — just noting the cost.

Test Coverage

5. test_insert_retry_wraps_each_attempt_in_requires_new_transaction only checks that requires_new appears, not that each retry uses it

assert race_model.transaction_options_seen.any? { |options| options[:requires_new] }

For a no-collision case (one successful INSERT), this still passes because the first attempt also runs inside requires_new. The test doesn't validate the retry path specifically. A stronger assertion would check that requires_new appears at least as many times as there are retry attempts.

6. test_not_null_slug_model_still_generates_unique_slugs_for_duplicate_titles uses 20 sequential creates

This is a good integration test. One note: for StrictSlugModel, compute_slug is called in before_validation before the record has an ID. The uniqueness check inside generate_unique_slug uses self.class.exists?(slug: ...) which queries the DB — this is correct, but each of those 20 creates does one or more DB queries during validation. Worth being aware that the NOT NULL path is more query-heavy at create time than the nullable path.

7. No test for the edge case where around_create retry succeeds but after_create :set_slug then double-fires

For StrictSlugModel, after a successful INSERT retry, after_create :set_slug runs. set_slug calls set_slug_with_retry, which returns early because slug.blank? is false (slug was set in before_validation). This is correct. But there is no test that explicitly asserts the slug value after an INSERT-time retry equals the retried slug, not some other value. test_insert_time_slug_collision_retries_and_succeeds asserts record.slug.start_with?("acme") which covers this implicitly, but a more explicit assertion would help.

Minor Notes

8. README change softens a hard warning

The old text was clear:

Your slug attribute SHOULD NOT have null: false

The new text is more nuanced and accurate, but beginners may miss that null: false requires an explicit before_validation callback on their part. Consider adding a short code example for the null: false path in the README.

9. StrictSlugModel definition in test_helper.rb is permanent global state

Since strict_slug_models is a schema migration run once, and StrictSlugModel is a top-level class, any test file loaded after test_helper.rb gets this model. That's fine for the current test suite, but it's worth documenting in test_helper.rb that this model is specifically for insert-time race condition tests.

Summary

The two blocking issues worth addressing before merge are items 1 (ID-based retry futility) and 3 (false-positive direction of the string match). Everything else is a suggestion.

CLAUDE.md

This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository.

Please go ahead and read the full context for this project at .cursor/rules/0-overview.mdc and .cursor/rules/1-quality.mdc now. Also read the README for a good overview of the project.

[Unreleased]

• Added retry handling for ActiveRecord::RecordNotUnique slug collisions during post-create slug persistence
• Added around_create retry handling for insert-time slug collisions in pre-insert (null: false) slug strategies
• Wrapped insert retries in savepoint transactions (requires_new: true) to keep PostgreSQL transactions retry-safe
• Added regression coverage for insert-time and update-time slug race windows

[0.2.0] - 2026-01-16

• Added a full Minitest test suite
• Fixed respond_to?(:slug) returning false for models using method_missing by implementing respond_to_missing?
• Fixed collision resolution generating identical suffixes by switching from id-based deterministic suffixes to SecureRandom-based random suffixes
• Fixed length parameter edge cases (zero, negative, very large values) by adding length validation and clamping via normalize_length method
• Fixed timestamp fallback to include random suffix for additional uniqueness guarantee
• Added length validation constants (MAX_HEX_STRING_LENGTH, MAX_NUMBER_LENGTH) to prevent invalid length values

[0.1.1] - 2024-03-21

• Slugs can be now generated based off methods that return a string, not just based off attributes
• Enhanced collision resolution strategy so that it doesn't get stuck in infinite loops
• Added comprehensive test suite

[0.1.0] - 2024-10-30

• Initial release

test2

test edit

[Unreleased]

• Added retry handling for ActiveRecord::RecordNotUnique slug collisions during post-create slug persistence
• Added around_create retry handling for insert-time slug collisions in pre-insert (null: false) slug strategies
• Wrapped insert retries in savepoint transactions (requires_new: true) to keep PostgreSQL transactions retry-safe
• Added regression coverage for insert-time and update-time slug race windows

[0.2.0] - 2026-01-16

• Added a full Minitest test suite
• Fixed respond_to?(:slug) returning false for models using method_missing by implementing respond_to_missing?
• Fixed collision resolution generating identical suffixes by switching from id-based deterministic suffixes to SecureRandom-based random suffixes
• Fixed length parameter edge cases (zero, negative, very large values) by adding length validation and clamping via normalize_length method
• Fixed timestamp fallback to include random suffix for additional uniqueness guarantee
• Added length validation constants (MAX_HEX_STRING_LENGTH, MAX_NUMBER_LENGTH) to prevent invalid length values

[0.1.1] - 2024-03-21

• Slugs can be now generated based off methods that return a string, not just based off attributes
• Enhanced collision resolution strategy so that it doesn't get stuck in infinite loops
• Added comprehensive test suite

[0.1.0] - 2024-10-30

• Initial release

PR Review: Race condition retry mechanism

The PR addresses a real concurrency problem and is generally well-structured. The separation between INSERT-time retries (retry_create_on_slug_unique_violation) and post-create retries (set_slug_with_retry) is clear and the test coverage is thorough. That said, there are a few issues worth addressing before merging.

Critical: set_slug_with_retry is broken on PostgreSQL

retry_create_on_slug_unique_violation correctly wraps each attempt in a savepoint (transaction(requires_new: true)), but set_slug_with_retry does not:

def set_slug_with_retry
  return unless slug.blank?

  with_slug_retry(-> { self.slug = nil }) do
    self.slug = compute_slug
    self.save   # no savepoint
  end
end

In PostgreSQL, a RecordNotUnique error inside an active transaction puts that transaction into an aborted state. Any subsequent SQL executed in the same transaction (including the retry's self.save) will fail immediately with "current transaction is aborted, commands ignored until end of transaction block". The retry loop in with_slug_retry would always re-raise on the second attempt.

set_slug_with_retry is called from after_create :set_slug, which runs inside the transaction opened by save!. A savepoint is needed here too:

def set_slug_with_retry
  return unless slug.blank?

  with_slug_retry(-> { self.slug = nil }) do
    self.slug = compute_slug
    self.class.transaction(requires_new: true) { self.save }
  end
end

Because the test suite uses an in-memory SQLite database, this bug is not caught — SQLite is more lenient about reusing a transaction after a constraint failure. A PostgreSQL integration test would expose it immediately.

Moderate: Unnecessary savepoint overhead for nullable slug models

retry_create_on_slug_unique_violation only guards against INSERT-time slug collisions, which can only happen when the slug column is NOT NULL (i.e., the slug must be committed in the same INSERT). For nullable slug models (the default), slug is NULL at INSERT time, so a slug unique violation at INSERT is impossible.

Despite this, slug_persisted? returns true for all models with a slug column (both nullable and non-nullable), so every create call on a normal TestModel now opens and closes a savepoint — pure overhead that never helps.

One approach is to skip the savepoint path for nullable columns by inspecting self.class.columns_hash["slug"]&.null == false. This keeps the common case (nullable slug) fast.

Moderate: slug_unique_violation? regex is fragile with PostgreSQL constraint names

The /\bslug\b/ word-boundary regex won't match constraint names like index_users_on_slug because _ is a \w character — there is no word boundary between _ and s in on_slug. For SQLite and MySQL, the error messages include the column name (e.g., UNIQUE constraint failed: test_models.slug), where slug is preceded by . (non-word), so \b matches fine. For PostgreSQL, the error message includes the constraint name (e.g., "index_users_on_slug"), where \bslug\b would not match.

In practice, detection probably still works on PostgreSQL because the error's DETAIL line typically reads Key (slug)=(value) already exists., and (slug) gives you the word boundaries. But this is an implicit dependency on a specific message format. Consider adding an explicit test with a constraint-name-style error string that resembles what pg actually produces:

pg_error = ActiveRecord::RecordNotUnique.new(
  'PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "index_users_on_slug"'
)
# Verify slug_unique_violation? correctly identifies this

Minor: Misleading test comment in test_insert_time_slug_collision_retries_and_succeeds

assert_equal 2, race_model.compute_slug_calls,
  "expected compute_slug in before_validation and again before retry"

The second compute_slug call comes from compute_slug_for_retry inside pre_retry_action, not from before_validation re-running. around_create's retry does not re-execute validation callbacks. The comment should read something like "expected compute_slug in before_validation (initial) and once via compute_slug_for_retry (retry action)".

Minor: compute_slug_for_retry changes slug format for ID-based strategies

For ID-based strategies, the slug changes from e.g. d4735e3a265 to d4735e3a265-481923. As the comments note, ID-based slug collisions are theoretically impossible (two records cannot share an ID), so this path is dead code in practice. But if it ever did execute, users would see slugs in an unexpected format. Worth a comment noting this is defensive dead code, or alternatively raising a descriptive error instead of silently producing a malformed slug.

What is done well

• The core design (two separate retry layers for two distinct race scenarios) is sound and correctly reasoned.
• retry_if: ->(_error) { !persisted? } cleanly prevents around_create from double-retrying failures that already originated from the after-create UPDATE path.
• MAX_SLUG_GENERATION_ATTEMPTS is respected consistently in both retry paths.
• Test coverage is solid; the simulated-collision patterns (injecting a competing row via raw SQL in a before_create hook) are a clean way to unit-test race behavior deterministically.
• Good documentation in inline comments explaining the trade-offs.

Code Review

This is a solid approach to a real problem — the race condition window between EXISTS? and INSERT is a known issue with optimistic slug generation. The two-path design (nullable vs. NOT NULL slug columns) is well-thought-out. Below are my findings ordered by severity.

Bug / Misleading Comment: \bslug\b does not match index_on_slug

lib/slugifiable/model.rb:248-252

The comment says:

"reslugged_items" won't match, but "slug" and "index_on_slug" will

This is incorrect. In Ruby regex, \w includes underscore (_), so \b does not fire between _ and s in "index_on_slug". The pattern \bslug\b would not match "index_users_on_slug".

"index_on_slug".match?(/\bslug\b/)  # => false (underscore is \w)
"test_models.slug".match?(/\bslug\b/) # => true (period is \W)

In practice this is probably fine because:

• SQLite errors: "UNIQUE constraint failed: test_models.slug" — the . before slug provides the word boundary, so it matches ✓
• PostgreSQL errors typically include DETAIL: Key (slug)=(value) — the ( provides the word boundary ✓
• MySQL errors contain the column/key name directly ✓

But the comment should be corrected to avoid future confusion, and it's worth adding a test for a PostgreSQL-style constraint name like "index_users_on_slug" to confirm behavior:

pg_error = ActiveRecord::RecordNotUnique.new(
  'PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "index_users_on_slug"\nDETAIL:  Key (slug)=(my-slug) already exists.'
)
assert model.send(:slug_unique_violation?, pg_error)

Design Concern: around_create double-yield is non-standard Rails usage

lib/slugifiable/model.rb:275

Calling yield multiple times inside an around_create callback (as with_slug_retry does via retry) is undocumented Rails behavior. The yield in around_create is internally a callable that wraps the save + subsequent callbacks. Invoking it repeatedly works today (the tests confirm this), but it is not a documented contract from Rails and could silently change behavior in a future major version.

An alternative to consider would be a before_create callback that sets a pre-computed slug, combined with an explicit save rescue/retry loop in a class-level create override. However, that would be significantly more invasive. The around_create approach is pragmatic for a library gem, and adding a comment acknowledging this would help future maintainers:

# NOTE: This calls `yield` multiple times, once per attempt. This relies on
# Rails `around_create` yielding a re-invocable Proc. Tested against Rails 6-8.

Minor: set_slug_with_retry silently dropped the id_changed? guard

Old set_slug:

self.slug = compute_slug if id_changed? || slug.blank?
self.save

New set_slug_with_retry:

return unless slug.blank?

The id_changed? branch was removed. In practice this is fine — after create, id_changed? is always true, but slug.blank? is also always true for new nullable-slug records, so the conditions were redundant. However, the original code also had a bug: it called self.save unconditionally even when neither condition was true. The new code fixes this implicitly. Worth calling out as an intentional fix in the PR description or a comment.

Minor: test_update_slug_if_nil_uses_retry_mechanism doesn't test retry

test/slugifiable/race_condition_retry_test.rb:160-170

The test name implies the retry mechanism is exercised, but there's no collision — it simply tests that update_slug_if_nil regenerates a nil slug. The retry path is not covered here. The test is still useful, but renaming it to test_update_slug_if_nil_regenerates_slug would be more accurate.

Minor: compute_slug_for_retry ID-based path could produce malformed slugs

lib/slugifiable/model.rb:294-298

The comment correctly labels the ID-based path as "defensive dead code," but if it were ever reached (e.g., if someone customizes ID slug generation to be non-unique), the output would be "<hex_hash>-<random_number>" — mixing a hex hash with a decimal suffix. The comment is good, but consider whether this path should raise NotImplementedError instead of silently producing an unusual format. At minimum, the "dead code" acknowledgement in the comment is appreciated.

Positive observations

• The retry_if: ->(_error) { !persisted? } guard cleanly separates INSERT-time failures from after_create failures without needing to track which callback raised. Elegant.
• Wrapping each attempt in requires_new: true (savepoint) is the correct approach for PostgreSQL — without this, the outer transaction would be aborted on the first constraint violation.
• Adding the missing add_index :test_models, :slug, unique: true to test_helper.rb means tests now catch DB-level constraint violations, not just AR validation failures. This is an important improvement to test fidelity.
• The two-path design (nullable slug via after_create retry, NOT NULL slug via around_create retry) is well-separated and the interaction between them is sound.
• StrictSlugModel in test_helper.rb gives a clear, realistic model of the NOT NULL use case.
• Test coverage for the retry limit, non-slug bubble-up, and slug clearing before regeneration are all the right cases to cover.

Summary

The core logic is correct and well-tested. The main actionable items are:

1. Fix the misleading regex comment in slug_unique_violation? and add a PostgreSQL-style constraint name test case
2. Add a comment on the around_create double-yield acknowledging the Rails internals dependency
3. Rename test_update_slug_if_nil_uses_retry_mechanism to reflect what it actually tests

PR Review: Fix race condition in slug generation with retry mechanism

Overall this is a well-thought-out fix for a real problem. The design is solid — wrapping saves in savepoints for PostgreSQL correctness is the right approach, and the two-path strategy (INSERT-time vs. post-create) addresses both NULL and NOT NULL schema configurations. A few things worth discussing:

Bug: slug_unique_violation? regex may miss MySQL violations

lib/slugifiable/model.rb

def slug_unique_violation?(error)
  message = error.message.to_s.downcase
  cause_message = error.cause&.message.to_s.downcase
  [message, cause_message].any? { |m| m.match?(/\bslug\b/) }
end

The \b word boundary won't match when slug is preceded by _, because Ruby regex treats _ as a word character. MySQL's duplicate key error format is:

Duplicate entry 'foo' for key 'index_posts_on_slug'

Here \bslug\b won't match on_slug (no word boundary between _ and s). This means on MySQL with a conventionally-named index, the exception would bubble up as an unhandled error instead of triggering a retry.

The inline comment says "MySQL: includes column name directly" but that's only true if the index name happens to be slug rather than the Rails default index_table_name_on_slug.

A more robust detection approach might check the column name from error.cause (e.g., PG::UniqueViolation exposes #column_name via the result) or use a broader pattern like /\bslug\b|[_(]slug[_)]/. Alternatively, document MySQL as unsupported for the retry path.

Fragile: re-invoking yield in around_create via retry

lib/slugifiable/model.rb, lines 266-282

def retry_create_on_slug_unique_violation
  # ...
  with_slug_retry(...) do
    self.class.transaction(requires_new: true) { yield }
  end
end

The comment acknowledges this: "This relies on Rails around_create yielding a re-invocable Proc, which is undocumented but has worked consistently in Rails 6-8."

A retry inside a rescue block re-executes the begin block, which calls yield again — and Rails' callback chain passes the next-callback invocation as that block. This works in practice but couples the implementation to Rails internals that could change.

A safer alternative would be to move retry logic into before_create (pre-compute slug + skip after_create re-computation) or rescue at the create\! call site instead. Worth at minimum verifying this across supported Rails versions in CI.

Dead code: compute_slug_for_retry ID-based path

lib/slugifiable/model.rb, lines 297-304

def compute_slug_for_retry
  base_slug = compute_slug
  if id_based_slug_strategy?
    "#{base_slug}-#{SecureRandom.random_number(10 ** DEFAULT_SLUG_NUMBER_LENGTH)}"
  else
    base_slug
  end
end

The comment correctly notes this is defensive dead code (two records can't share the same ID, so ID-based slug collisions are impossible). The extra method, the id_based_slug_strategy? helper, and the determine_slug_generation_method call it triggers all add complexity without benefit. Simplifying to just compute_slug directly would reduce surface area.

Subtle: with_slug_retry attempt counter naming

lib/slugifiable/model.rb, lines 318-333

attempts tracks failures, not total attempts. With MAX_SLUG_GENERATION_ATTEMPTS = 10, you get 10 total executions (1 initial + 9 retries). The variable name reads as "total attempts" when it actually counts failures. Renaming to failures or retries would better reflect intent and make the limit check raise if failures >= MAX_SLUG_GENERATION_ATTEMPTS more intuitive.

Good: add_index :test_models, :slug, unique: true addition

Adding the unique index to the test schema is necessary for the retry tests to actually exercise the constraint. This is a behavior-altering change to the existing test suite — any test that previously created two records with the same slug would now fail at the DB level. Existing tests apparently don't do this (they pass), but worth noting.

Good: savepoint transaction wrapping

Using requires_new: true is the correct approach for PostgreSQL, where a RecordNotUnique inside a transaction leaves the outer transaction in an aborted state, making any subsequent operation fail. The savepoint isolates the failing attempt so the outer transaction remains usable.

Minor: StrictSlugModel slug column has no database default

test/test_helper.rb, lines 49-54

With null: false and no database default, any subclass that doesn't set a slug before INSERT gets a PG::NotNullViolation rather than a RecordNotUnique. This is fine for the test model but worth noting in docs for users attempting the NOT NULL setup: the before_validation callback is required, not optional.

Summary

Code Review

Overall this is a well-motivated fix for a real race condition. The problem description is accurate, the solution is structured clearly, and the test coverage is extensive. Below are the issues I found, ordered by severity.

Bug: id_changed? check silently dropped

lib/slugifiable/model.rb:228-229

The original set_slug was:

self.slug = compute_slug if id_changed? || slug.blank?
self.save

The new set_slug_with_retry is:

return unless slug.blank?

The id_changed? branch has been quietly removed. For attribute-based slugs this is fine (the ID never changes after creation), but for ID-based slug strategies, compute_slug_as_string hashes self.id. If anything ever triggered a re-run of set_slug with a different ID in the future, the slug would not be updated. This is almost certainly dead code in practice, but silently dropping the check changes the contract and makes the diff harder to reason about. Either restore it or add a comment explaining why it was intentionally removed.

Fragile: around_create re-yield via retry is undocumented Rails behavior

lib/slugifiable/model.rb:266-282

The code itself calls this out:

# NOTE: This calls `yield` multiple times (once per attempt) via `retry`.
# This relies on Rails `around_create` yielding a re-invocable Proc, which
# is undocumented but has worked consistently in Rails 6-8.

This is the most significant design concern. Rails' callback internals use ActiveSupport::Callbacks::CallbackChain which builds a composed method chain around the core operation. There is no public guarantee that calling yield a second time from an around_create block is safe. The risks include:

• Dirty tracking state not resetting between attempts (id_was, slug_was, etc.)
• new_record? becoming stale after the first failed persist attempt
• Rails' internal bookkeeping for @new_record / @persisted being set after the first yield, making the second yield behave as an update rather than an insert

A safer approach that avoids this entirely: instead of around_create, use before_create to compute and set the slug (what StrictSlugModel.ensure_slug_for_insert already does), and then catch RecordNotUnique in an after_rollback or rescue in a service layer. Alternatively, the before_create callback itself could be wrapped in a retry loop without ever re-yielding.

If you keep the current approach, it should at minimum have an integration test that actually hits the database with a real concurrent INSERT, not just one that injects a raw SQL insert in a before_create. The existing test is valuable but does not exercise the full Rails state machine the way a real concurrent request would.

Bug: slug_unique_violation? has false positives on _slug-suffixed columns

lib/slugifiable/model.rb:256

pattern = /\bslug\b|_slug\b/

The _slug\b branch is intended to catch MySQL messages like "for key 'index_posts_on_slug'". But it also matches any column whose name ends in _slug: canonical_slug, parent_slug, user_slug, original_slug, etc. If an app has a unique index on canonical_slug and two records race to save the same value, the retry mechanism would incorrectly fire — clearing the slug column and retrying — when the actual collision is on a completely different column.

A tighter pattern would anchor more precisely to the index name convention:

pattern = /\bslug\b|\bon_slug\b/

Or, rather than substring matching at all, inspect error.cause for the actual column name via the adapter's TranslationContext (available in Rails 6.1+). That is more work but eliminates the false-positive class entirely.

Minor: test_insert_retry_hook_exists tests private method existence

test/slugifiable/insert_race_retry_test.rb:15-18

assert model.respond_to?(:retry_create_on_slug_unique_violation, true)

Testing that an internal private method exists by name is brittle — it will break on any rename and doesn't verify behavior. The method's behavior is already covered by test_insert_time_slug_collision_retries_and_succeeds. Consider removing this test or converting it to test observable behavior (e.g., that creating two records with the same title succeeds without raising).

Minor: compute_slug_for_retry is acknowledged dead code but still ships

lib/slugifiable/model.rb:288-303

The comment reads:

# NOTE: The ID-based path is defensive dead code — ID-based collisions are
# impossible since two records can't share the same ID. If this path were
# ever triggered, the slug format would change (e.g., "abc123" -> "abc123-481923").

Shipping code that is documented as dead and that changes the observable slug format if somehow reached is a smell. If it truly cannot fire, the if id_based_slug_strategy? branch can be removed — making the method a simple compute_slug call — without changing any externally observable behavior. Dead branches in a library add maintenance surface without value.

Minor: slug_column_not_null? is checked on every create

lib/slugifiable/model.rb:284-286

def slug_column_not_null?
  self.class.columns_hash["slug"]&.null == false
end

This is called inside retry_create_on_slug_unique_violation, which runs as an around_create callback for every record creation. columns_hash is cached by Rails' schema cache so this is not a repeated DB round-trip, but it does execute on every create even for models with nullable slug columns. A one-time memoization on the class level (e.g., @slug_column_not_null set in included) would be cleaner and make the per-create cost more explicit.

Nit: Missing unique index was a pre-existing gap

test/test_helper.rb:42

add_index :test_models, :slug, unique: true

This is a good addition. Worth calling out in the CHANGELOG since it changes the behavior of the test suite and reflects what production setups require to actually enforce uniqueness at the DB level.

Summary

The retry logic in with_slug_retry itself is clean, the savepoint wrapping is correct and necessary for PostgreSQL, and the MAX_SLUG_GENERATION_ATTEMPTS boundary condition is right. The two test files are well-structured and the simulation approach using before_create injection is clever. Addressing the around_create re-yield concern is the highest-priority item before merge.

Code Review

Overall this is a well-structured fix for a real race condition. The problem is correctly diagnosed, the solution is layered appropriately, and the test coverage is thorough. A few items worth discussing before merging:

Behavioral Change in set_slug (potential regression)

The old set_slug was:

self.slug = compute_slug if id_changed? || slug.blank?
self.save

The new path (via set_slug_with_retry) is:

return unless slug.blank?

The id_changed? guard is gone. In an after_create context id_changed? is always true, so previously the slug would be recomputed even if it was non-blank (e.g., set in a before_save). Now it's skipped. This is the correct behavior for the null: false case, but it is a silent behavior change for anyone relying on the old overwrite semantics. Consider noting it in the CHANGELOG or at minimum in a comment.

Relying on undocumented Rails behavior (around_create + retry)

# NOTE: This calls `yield` multiple times (once per attempt) via `retry`.
# This relies on Rails `around_create` yielding a re-invocable Proc, which
# is undocumented but has worked consistently in Rails 6-8.
def retry_create_on_slug_unique_violation

The PR already calls this out — good. But calling yield a second time inside around_create via retry re-runs the entire create chain (validations, before_create, INSERT, after_create). This works today because Rails builds callback chains as re-callable Procs/lambdas, but it is not guaranteed by the Rails API contract and could break silently in a future version. The existing Appraisals matrix (Rails 7.2 / 8.0 / 8.1) helps, but a comment pointing to a specific Rails source revision would make this easier to audit on upgrades.

Exception propagation after max retries from after_create

generate_unique_slug has a timestamp-based fallback after MAX_SLUG_GENERATION_ATTEMPTS:

slug_candidate = "\#{base_slug}-\#{Time.current.to_i}-\#{SecureRandom.random_number(1000)}"

with_slug_retry raises instead:

raise if attempts >= MAX_SLUG_GENERATION_ATTEMPTS

For the after_create path, an unhandled raise inside the callback rolls back the outer transaction — so 10 consecutive race-condition failures will cause create\! to blow up rather than fall back gracefully. This is a breaking change in the error behavior that isn't mentioned in the CHANGELOG. If you want parity with the existing lenient fallback, set_slug_with_retry could rescue after max retries and apply the timestamp suffix rather than re-raise.

slug_unique_violation? — non-standard index names

The pattern /\bslug\b|_on_slug\b/ correctly handles SQLite, PostgreSQL, and MySQL with Rails-generated index names. One unaddressed edge case: custom index names like uq_slug_col, slug_idx, or posts_slug_unique won't match either branch. The consequence is a false negative (the exception bubbles up, which is the safe outcome). Worth a brief comment noting this limitation so future maintainers know why retries won't fire for a custom schema.

Minor nits

compute_slug_for_retry — dead code path

# NOTE: The ID-based path is defensive dead code
def compute_slug_for_retry
  base_slug = compute_slug
  if id_based_slug_strategy?
    "\#{base_slug}-\#{SecureRandom.random_number(10 ** DEFAULT_SLUG_NUMBER_LENGTH)}"
  else
    base_slug
  end
end

If the ID-based path truly cannot be triggered, the method could just return compute_slug and the id_based_slug_strategy? helper becomes dead code too. If keeping it for defensive future-proofing, make that intent explicit.

slug_column_not_null? called on every create

retry_create_on_slug_unique_violation hits columns_hash on every record creation. columns_hash is AR-cached so it's cheap, but a class-level memoized check (e.g., @slug_column_not_null ||= ...) would make the intent clearer.

test_insert_time_slug_collision_retries_and_succeeds assertion is fragile

assert_equal 2, race_model.compute_slug_calls,
  "expected compute_slug in before_validation and again before retry"

This ties the test to internal call counts. If compute_slug is ever called from another hook, the test breaks even though behavior is correct. Testing outcomes (unique slug assigned, persisted? is true) is more durable.

Test coverage

The four new test files are genuinely good regression coverage. The cross-adapter slug_unique_violation_detection_test.rb is especially valuable. The state machine tests directly address the multi-yield concern with concrete assertions. No gaps that would block merging.

Summary: The core fix is sound and the approach is well-reasoned. The two items I'd prioritize before merging: (1) document the around_create multi-yield reliance more explicitly, ideally with a test that would fail on Rails upgrade if the behavior changes; and (2) decide intentionally whether max-retry exhaustion in after_create should raise or fall back to the existing timestamp suffix. Everything else is polish.

PR Review: Race Condition Fix with Retry Mechanism

This is a well-motivated change — RecordNotUnique slug collisions crashing in production is a real problem. The implementation is thoughtful and the test suite is extensive. Below is detailed feedback.

Summary

The PR adds two retry paths:

1. Post-create UPDATE path (after_create :set_slug → set_slug_with_retry): catches RecordNotUnique on the UPDATE, retries up to MAX_SLUG_GENERATION_ATTEMPTS times, falls back to a timestamp suffix on exhaustion.
2. INSERT-time path (around_create :retry_create_on_slug_unique_violation): for NOT NULL slug columns where the slug is set before INSERT, retries the entire create on collision, raises on exhaustion.

Bugs / Correctness

1. around_create relies on undocumented Rails behavior (most significant concern)

Calling retry inside an around_create while invoking yield multiple times works in Rails 6-8 today, but this is explicitly not part of the public API. Rails callbacks are internally represented as re-callable Procs, and this could change without a deprecation notice.

The PR acknowledges this with a comment and uses around_create_state_machine_test.rb as a canary, which is a reasonable mitigation. But it remains a real maintenance risk on Rails upgrades. Suggest either:

• Adding a versioned guard (raise or log a warning on Rails versions where this is known to break), or
• Replacing retry / re-yield with a more stable pattern: build the full save logic into the retry loop without re-invoking yield

2. Silent failure in on_exhaustion

In set_slug_with_retry, the final exhaustion fallback does:

self.class.transaction(requires_new: true) { self.save }

self.save (not save!) returns false on failure without raising. If the timestamp-suffixed slug also collides (extremely unlikely but possible), the record ends up persisted with slug = nil and no error is raised anywhere. Consider using save! here, or at minimum raising when save returns false.

3. id_changed? guard removed without explanation

The original set_slug had if id_changed? || slug.blank?. The new version only checks slug.blank?. This is probably correct (since update_slug_if_nil already gates on slug.nil?), but the removal is uncommented. Please add a note explaining why the id_changed? check was intentionally dropped.

Code Quality

4. compute_slug_for_retry is a thin no-op wrapper

def compute_slug_for_retry
  compute_slug
end

This adds an indirection layer with no functional value. The intent ("ID-based retries are not possible at this stage") is better expressed as a comment at the compute_slug call site in with_slug_retry.

5. slug_column_not_null? should be class-level memoization

def slug_column_not_null?
  @slug_column_not_null ||= self.class.columns_hash["slug"]&.null == false
end

@slug_column_not_null is an instance variable, so the hash lookup runs on the first access of every new instance. Column nullability is a schema-level (class-level) property and should be memoized at the class level via a class method or class_attribute.

6. on_exhaustion calls compute_slug with self.slug = nil

In the exhaustion lambda, self.slug is nil (cleared by the last retry's pre_retry_action). compute_slug calls generate_unique_slug, which does up to 10 EXISTS? database queries — only for the result to be immediately overwritten with a timestamp suffix. Cache the base slug before entering the retry loop:

base_slug = compute_slug  # compute once, before the loop
on_exhaustion = -> {
  self.slug = "#{base_slug}-#{Time.current.to_i}-#{SecureRandom.random_number(1000)}"
  self.class.transaction(requires_new: true) { self.save\! }
}

Performance

7. Savepoint overhead on every create

set_slug_with_retry wraps every slug save in transaction(requires_new: true), creating a SAVEPOINT/RELEASE SAVEPOINT round-trip even on the happy path with no outer transaction and no contention. For high-throughput write paths this adds measurable overhead in PostgreSQL. Consider skipping the savepoint when no outer transaction is open:

if self.class.connection.transaction_open?
  self.class.transaction(requires_new: true) { self.save }
else
  self.save
end

Test Coverage

The test suite is thorough: 42 new tests across 4 files covering violation detection, INSERT-time retry, post-create UPDATE retry, and Rails state machine integrity. A few gaps:

8. No test for silent failure in on_exhaustion

There is no test asserting that exhaustion of set_slug_with_retry when the timestamp fallback also fails produces a predictable outcome. With the current self.save (not save!), the failure is silent.

9. Test name mismatch

test_sequential_creates_with_same_title is documented as "concurrent creates (simulated)" but is sequential. The name should not include "concurrent" to avoid misleading future maintainers.

10. compute_slug_calls == 2 assertion is fragile

In test_insert_time_slug_collision_retries_and_succeeds, the assertion that compute_slug is called exactly 2 times is tied to undocumented Rails behavior about when before_validation re-runs during retry-inside-around_create. If validation re-runs (which it does, since yield re-runs the full create lifecycle), before_validation fires again, potentially inflating the count. This test may give false confidence.

Bottom Line

The core idea is sound. The two issues worth resolving before merge:

1. The undocumented around_create + retry + yield pattern — the canary test is a good mitigation, but also consider adding a Rails version guard, or replacing it with a more stable implementation that does not rely on re-invoking yield.
2. The silent save failure in on_exhaustion — use save! or explicitly raise/log when the fallback save returns false.

The rest of the feedback is lower priority but worth addressing for code clarity and long-term maintainability.

PR Review: Fix race condition in slug generation with retry mechanism

Good overall approach to a real problem. The savepoint wrapping for PostgreSQL transaction safety is the right call, and the test coverage across different DB error formats is thorough. A few issues worth addressing before merging:

Bugs / Behavioral Changes

1. id_changed? check silently dropped from set_slug

The original:

self.slug = compute_slug if id_changed? || slug.blank?
self.save

The new set_slug_with_retry only guards on slug.blank?. While ID changes are rare in practice, this is a behavioral regression for any model using ID-based slug generation with a mutable PK (e.g., custom string PKs, UUID PKs assigned externally). At a minimum this should be documented in CHANGELOG.

2. on_exhaustion calls compute_slug and then appends another timestamp suffix

In set_slug_with_retry:

on_exhaustion = -> {
  base_slug = compute_slug  # already calls generate_unique_slug which appends random suffixes
  self.slug = "#{base_slug}-#{Time.current.to_i}-#{SecureRandom.random_number(1000)}"
  ...
}

compute_slug already calls generate_unique_slug which does EXISTS checks and falls back to a timestamp suffix after MAX_SLUG_GENERATION_ATTEMPTS attempts. The exhaustion handler then appends yet another timestamp+random on top, potentially producing double-suffixed slugs like "my-title-123456-1704067200-456". The intent should be to append the timestamp to the raw parameterized base, not to an already-resolved slug.

Fragile Rails Internal

3. Calling yield multiple times inside around_create via retry relies on undocumented behavior

The PR acknowledges this explicitly:

This relies on Rails around_create yielding a re-invocable Proc, which is undocumented but has worked consistently in Rails 6-8.

The around_create_state_machine_test.rb mitigates the risk, but this is worth noting prominently in CHANGELOG as a known fragility. If Rails ever changes how around_ callbacks build their Proc chains, this could break silently on a Rails upgrade.

Design Concerns

4. compute_slug_for_retry is a transparent wrapper with no behavior

def compute_slug_for_retry
  compute_slug
end

This is fine as an extension point for subclasses, but currently dead abstraction. If the intent is to allow overriding retry slug generation, a comment explaining the hook purpose would be clearer than the current docstring (which just re-describes what compute_slug does).

5. slug_column_not_null? memoizes per-instance instead of per-class

Schema metadata does not change per instance. Using @slug_column_not_null works (and the defined? idiom is correct for boolean memoization), but class-level caching would avoid repeated columns_hash lookups across instances. Minor in practice since columns_hash is already cached by Rails, but semantically cleaner.

Minor Issues

6. gem "pg" added to Gemfile without version constraint or platform guard

This requires PostgreSQL client libraries in every dev/CI environment even though the tests that use it skip when SLUGIFIABLE_TEST_POSTGRES_URL is absent. Consider removing it from Gemfile entirely since pg is already conditionally required inside the test itself (with rescue LoadError).

7. The test_max_retries_exceeded_raises_error assertion tests internal invocation count

# MAX retries + 1 fallback attempt with timestamp suffix
assert_equal Slugifiable::Model::MAX_SLUG_GENERATION_ATTEMPTS + 1, race_model.update_attempts

This is correct (10 retries + 1 exhaustion attempt), but tests internal invocation count rather than observable behavior. If the exhaustion strategy changes (e.g., generating the fallback slug differently), this test would break for the wrong reason. Testing that the error is raised and no record persists is more robust.

What's working well

• The slug_unique_violation? pattern /\bslug\b|_on_slug\b/ correctly handles false-positive columns (canonical_slug, parent_slug, etc.) — since _ is a word character in regex, \bslug\b won't match within original_slug. The test coverage for this is excellent.
• Using requires_new: true savepoints is the correct approach for keeping the outer PostgreSQL transaction healthy after a unique violation.
• The retry_if: ->(_error) { !persisted? } guard in retry_create_on_slug_unique_violation correctly prevents the outer around_create from retrying exceptions that actually originated from after_create :set_slug.
• The optional PostgreSQL integration test is a thoughtful addition for people who want to verify transaction-abort safety.

Summary: The two behavioral issues (#1 and #2) should be addressed before merging. The rest are design improvements worth considering.

PR Review: Fix race condition in slug generation with retry mechanism

This is a well-thought-out PR that addresses a real concurrency problem. The code is heavily documented and has strong test coverage. Below is my analysis.

Significant Concerns

1. before_create callbacks fire multiple times for NOT NULL slug models

The retry_create_on_slug_unique_violation around_create callback retries by calling yield again via Ruby's retry. Because around_create's yield re-executes the full inner callback chain (confirmed by around_create_state_machine_test.rb asserting states_seen.length == 2 from a before_create callback), any user-defined before_create callback will fire once per retry attempt.

Users with non-idempotent before_create callbacks — sending emails, enqueuing jobs, making external API calls, incrementing counters — will see those callbacks fire multiple times on a slug collision. The gem currently gives no indication of this behavior. This should be prominently documented in the README under the null: false section.

# This fires TWICE if a slug collision requires one retry
before_create do
  UserMailer.welcome(self).deliver_later  # Unexpected: user gets 2 emails
  Analytics.track("record_created")       # Unexpected: double-counted
end

The tests in around_create_state_machine_test.rb actually confirm this behavior, but from the outside it looks like a regression/surprise rather than an expected contract.

2. Relies on undocumented Rails behavior

The code explicitly acknowledges this in comments:

This relies on Rails around_create yielding a re-invocable Proc, which is undocumented but has worked consistently in Rails 6–8.

The around_create_state_machine_test.rb guards against regressions on upgrade, which is the right approach. Still, building a feature on undocumented behavior creates a maintenance burden: every Rails upgrade becomes a correctness risk until the test suite is confirmed to pass. This is acceptable pragmatically, but worth flagging for contributors.

Code Quality Issues

3. One test inspects source code instead of behavior

test_exhaustion_fallback_raises_on_validation_error in race_condition_retry_test.rb (lines ~1195–1219) reads the gem's source file and uses regex to confirm save! is present:

source = File.read(file_path)
assert_match(/on_exhaustion.*\{.*self\.save!.*\}/m, source, ...)
assert_match(/with_slug_retry.*do.*self\.save!.*end/m, source, ...)

This pattern tests implementation text, not behavior. It will break on any formatting change, whitespace change, or refactoring that doesn't alter behavior. The comment says triggering the actual behavior requires "complex setup with anonymous classes that have Rails naming issues" — but that complexity should be solved rather than avoided via source inspection. This test provides false confidence.

4. Inconsistency between compute_slug_based_on_attribute and compute_base_slug for :attribute_missing

When an attribute doesn't exist on the model:

• compute_slug_based_on_attribute returns compute_slug_as_string (a hex string like "d4735e3a265")
• compute_base_slug returns generate_random_number_based_on_id_hex (an Integer like 321678)

So the exhaustion fallback slug for a missing-attribute model would be "321678-1740000000-a1b2c3d4" rather than "d4735e3a265-1740000000-a1b2c3d4". The comment documents this as intentional ("All fallback cases use numeric ID-based slug for exhaustion path"), but it still means the slug format silently changes under exhaustion pressure for this edge case. If intentional, a brief note in the CHANGELOG or README edge cases section would help.

5. slug_column_not_null? class-level memoization could surprise users in development

klass.instance_variable_set(:@slug_column_not_null, klass.columns_hash["slug"]&.null == false)

This value is cached forever on the class. In development mode, if someone adds null: false to their slug column and runs db:migrate without restarting their server, the cache will hold the old false value. Rails' schema cache has the same issue, so this is not novel, but it's an additional layer of staleness. At minimum, a comment noting that this is development-mode-stale-friendly would be helpful.

Positive Aspects

• retry_if: ->(_error) { !persisted? } is a clever guard to prevent retrying INSERT when a RecordNotUnique is raised by an after_create callback on an already-persisted NOT NULL record. This edge case is easy to miss.

• slug_unique_violation? pattern is well-designed: \bslug\b correctly handles SQLite (table.slug) and PostgreSQL (Key (slug)=) formats, while _on_slug\b handles index names without false-positiving on canonical_slug, parent_slug, etc. The documented limitation (custom index names silently fail to retry) is the safe direction — false negative beats false positive here.

• Savepoint wrapping (requires_new: true) for each attempt is correct and necessary for PostgreSQL's transaction-abort behavior. The PostgreSQL integration test is a good addition.

• compute_slug_calls == 2 in test_insert_time_slug_collision_retries_and_succeeds correctly counts one call from before_validation :ensure_slug_for_insert and one from pre_retry_action. This would catch a regression if the retry mechanism stopped working.

• slug.blank? guard in set_slug_with_retry means the after_create path is harmless for NOT NULL slug models where slug is already set.

• Test files are well-organized into focused files per concern (slug_unique_violation_detection_test.rb, insert_race_retry_test.rb, around_create_state_machine_test.rb).

Summary

The implementation is correct and the core problem is well-solved. The highest priority item before merge is documenting the before_create re-execution behavior for NOT NULL slug models.

PR Review: Race condition fix with retry mechanism

Overall this is a well-considered fix for a real problem, with solid test coverage. The design is mostly sound but there are a few issues worth addressing before merging.

Critical

Reliance on undocumented around_create multi-yield behavior

retry_create_on_slug_unique_violation calls retry from inside an around_create callback, which re-invokes yield to re-run the full Rails callback chain. The code itself flags this:

# NOTE: This calls `yield` multiple times (once per attempt) via `retry`.
# This relies on Rails `around_create` yielding a re-invocable Proc, which
# is undocumented but has worked consistently in Rails 6-8.

The test in around_create_state_machine_test.rb is a good safety net, but this is still a fragile foundation — a Rails internals change could silently break the retry logic while all other tests pass. Consider whether a more explicit approach (e.g., a custom create!-wrapper or a helper module that implements its own save-with-retry loop) would be safer, even if more verbose.

Significant

save → save! is a breaking behavior change

set_slug_with_retry now uses save! inside a savepoint instead of the previous self.save:

# Before
self.save

# After
self.class.transaction(requires_new: true) { self.save! }

save! raises on any failure — not just RecordNotUnique. If an app has a validation that can fail during the after-create slug-save phase (e.g., a callback added by the consuming app that modifies state), the old code would silently skip saving the slug, while the new code raises ActiveRecord::RecordInvalid. This is a behavioral breaking change that should be called out in the CHANGELOG and documented clearly.

Asymmetric exhaustion handling

set_slug_with_retry (the after-create, nullable-slug path) falls back gracefully to a timestamp slug after exhaustion. retry_create_on_slug_unique_violation (the NOT NULL path) re-raises after exhaustion. This asymmetry means that a NOT NULL schema under sustained concurrent load will crash requests rather than degrade gracefully. The different choice is documented, but it will likely surprise users. At minimum, add a note to the README about this behavior difference.

Removed id_changed? check

The original set_slug was:

self.slug = compute_slug if id_changed? || slug.blank?
self.save

The id_changed? branch is gone. While this check was probably redundant for the after_create path specifically (where id always just changed from nil), its removal should be explicitly documented if it was intentional.

Minor

slug_column_not_null? is an instance method with class-level side effects

def slug_column_not_null?
  klass = self.class
  return klass.instance_variable_get(:@slug_column_not_null) if klass.instance_variable_defined?(:@slug_column_not_null)
  klass.instance_variable_set(:@slug_column_not_null, klass.columns_hash["slug"]&.null == false)
end

An instance method that writes to self.class instance variables is surprising and violates the principle of least astonishment. This should be a class method, e.g.:

def self.slug_column_not_null?
  @slug_column_not_null ||= columns_hash["slug"]&.null == false
end

(Note: @slug_column_not_null ||= false won't memoize a false result correctly — you'd need the instance_variable_defined? guard, which you already have in the instance method form.)

Misleading comment about validation re-runs on retry

In retry_create_on_slug_unique_violation:

# Recompute slug before retry because create-callback retries do not
# re-run validation callbacks.

This is incorrect. When retry causes yield to be re-called, before_validation does run again. The slug isn't recomputed during before_validation on retry because ensure_slug_for_insert checks slug.blank? and the slug was just set by compute_slug_for_retry. The comment should be corrected to avoid confusing future maintainers.

compute_base_slug fallback inconsistency with compute_slug_based_on_attribute

For the :attribute_missing case:

• compute_slug_based_on_attribute → compute_slug_as_string (hex string)
• compute_base_slug → generate_random_number_based_on_id_hex (integer)

So if retries are exhausted and the attribute is missing, the exhaustion slug uses a numeric base rather than the hex-string base that normal slug generation would produce. This is a minor inconsistency but worth noting in a comment.

Test using source file inspection is fragile

test_exhaustion_fallback_raises_on_validation_error reads the source file and regex-matches it to check that save! is used:

source = File.read(file_path)
assert_match(/on_exhaustion.*\{.*self\.save!.*\}/m, source, ...)

Source-file inspection tests are brittle — they break on any whitespace or refactoring change that doesn't affect behavior. Replace this with a behavioral test: configure a model with a validation that fails after MAX_SLUG_GENERATION_ATTEMPTS, assert that ActiveRecord::RecordInvalid is raised from the exhaustion path.

Positive notes

• The slug_unique_violation? pattern is well-thought-out with good false-positive prevention (the _on_slug\b vs _slug distinction), and the dedicated slug_unique_violation_detection_test.rb file with cross-adapter coverage is excellent.
• The parameterize_attribute_value refactor cleanly separates concerns and eliminates the duplicated attribute-lookup logic.
• The off == attempts fix for the >= condition in generate_unique_slug is a valid correctness improvement.
• Wrapping each attempt in requires_new: true to protect the outer PostgreSQL transaction is the right approach.
• The optional PostgreSQL integration test is a nice addition.

🤖 Generated with Claude Code

PR Review: Fix race condition in slug generation with retry mechanism

The core problem being solved is real and the overall approach is sound. Two hooks (around_create for INSERT-time collisions and after_create retry for post-insert slug assignment) give good coverage of different race windows. That said, there are a few correctness issues and code quality concerns worth addressing.

Correctness Issues

1. Redundant guard in retry_create_on_slug_unique_violation

self.slug = compute_slug if has_slug_method?

At this point, slug_persisted? has already been checked (it raises otherwise), and slug_persisted? is defined as has_slug_method? && self.attributes.include?("slug"). So has_slug_method? is always true here. The guard is dead code and suggests a misunderstanding of the control flow.

2. respond_to?(:id_changed?) guard is unnecessary

Inside after_create, id_changed? is always false (the record was just persisted and id is stable). The guard was presumably copied from before_create context. It should be removed to avoid confusion.

Design Concerns

3. slug_column_not_null? memoization is not invalidated on reset_column_information

klass.instance_variable_set(:@slug_column_not_null, ...)

If a developer runs migrations and calls reset_column_information, the cached value survives. This will cause incorrect behaviour in development when toggling the null constraint. Fix: override reset_column_information to clear @slug_column_not_null.

4. around_create runs for every model creation, even for nullable-slug models

The early-exit via return yield is correct for correctness, but the callback overhead (dispatch + slug_column_not_null? call + DB introspection on first call) affects every create. For nullable-slug models—the common case—this adds cost with no benefit.

Testing Concerns

5. test_exhaustion_fallback_raises_on_validation_error reads source code

source = File.read(file_path)
assert_match(/on_exhaustion.*\{.*self\.save!.*\}/m, source, ...)

This is fragile and doesn't test behaviour. A whitespace reformat breaks it. The actual behaviour (RecordInvalid propagates on exhaustion) should be tested directly by injecting a validation failure.

What's Well Done

• slug_unique_violation? word-boundary regex is correct and the false-positive tests for canonical_slug / parent_slug etc. are thorough.
• retry_if: ->(_error) { !persisted? } guard correctly avoids retrying when the error originated from an after_create on an already-persisted record.
• The around_create_state_machine_test.rb correctly guards against Rails removing the undocumented re-invocable Proc behaviour.
• SQL injection prevention via conn.quote() is correct.
• PostgreSQL integration test is properly opt-in.

Generated with Claude Code

Code Review

Overall this is a well-structured PR that solves a real race condition problem. The test coverage is extensive and the comments explaining the design decisions are thoughtful. Below are my findings.

🔴 Critical: Undocumented Breaking Change — save → save! in set_slug_with_retry

File: lib/slugifiable/model.rb, set_slug_with_retry

The original set_slug called self.save (silently returns false on failure). The new set_slug_with_retry calls self.save! inside a savepoint. This is a behavioral breaking change for all users of the nullable-slug path, not just the NOT NULL path:

# Before: silent failure
self.slug = compute_slug if id_changed? || slug.blank?
self.save  # returns false if validations fail post-create

# After: raises on any failure
self.class.transaction(requires_new: true) { self.save! }

Any app that has a validation that could fail between creation and the after_create callback (e.g. a uniqueness validator on a different column that races) will now get an unhandled ActiveRecord::RecordInvalid instead of a quiet no-op. The CHANGELOG's "Breaking Changes" section only calls out the before_create re-execution issue — this one should be listed explicitly there too.

🟡 Important: Relying on Undocumented Rails Behaviour for the around_create Retry

File: lib/slugifiable/model.rb, retry_create_on_slug_unique_violation (lines 330-350)

The comment acknowledges this:

This relies on Rails around_create yielding a re-invocable Proc, which is undocumented but has worked consistently in Rails 6-8.

Calling retry inside an around_create callback re-invokes yield a second time — this is genuinely undocumented and the Rails internals that make it work (callback chains built as re-callable lambdas) could change in a minor Rails release. The around_create_state_machine_test.rb will catch a regression, but only if the test suite is run on upgrade — it won't prevent a surprise breakage in production.

Worth noting in the gem's documentation that users of the NOT NULL path should run that specific test on every Rails upgrade.

🟡 Important: Class-Level Memoization of slug_column_not_null? Has No Invalidation

File: lib/slugifiable/model.rb, lines 354-358

def slug_column_not_null?
  klass = self.class
  return klass.instance_variable_get(:@slug_column_not_null) if klass.instance_variable_defined?(:@slug_column_not_null)
  klass.instance_variable_set(:@slug_column_not_null, klass.columns_hash["slug"]&.null == false)
end

Once set, this value is never cleared. Two scenarios where this causes incorrect behaviour:

1. Development code reloading: After a schema change (e.g. adding/removing null: false), ActiveRecord::Base.reset_column_information is called, but the cached @slug_column_not_null ivar on the class object will survive if the class is not fully reloaded.

2. Multi-database environments where the same class connects to different databases (e.g. Apartment/acts_as_tenant): the first connection's schema result is cached for all subsequent connections.

The simplest fix is to hook into reset_column_information:

def self.reset_column_information
  super
  @slug_column_not_null = nil
end

🟡 Important: around_create Overhead for All Models

File: lib/slugifiable/model.rb, line 41

retry_create_on_slug_unique_violation immediately bails with return yield for models without a NOT NULL slug column, so correctness is fine. However, every create call now pays the cost of an extra callback dispatch plus two DB-level checks (slug_persisted? and slug_column_not_null?). For models with nullable slugs — the documented default — this overhead is paid on every record creation with no benefit.

🟢 Minor: test_exhaustion_fallback_raises_on_validation_error Tests Source Code, Not Behaviour

File: test/slugifiable/race_condition_retry_test.rb, lines 1204-1228

source = File.read(file_path)
assert_match(/on_exhaustion.*\{.*self\.save!.*\}/m, source, ...)

This test is acknowledged as inspecting source instead of behaviour. It is fragile (a whitespace-only reformat breaks it) and tests the wrong thing. The behavioural outcome — that a RecordInvalid from the exhaustion save propagates — could instead be tested by injecting a validation failure in a subclass.

🟢 Minor: id_changed? Guard Dropped from set_slug

Before:

self.slug = compute_slug if id_changed? || slug.blank?
self.save

After:

return unless slug.blank?

The id_changed? guard would only matter if the ID changed after the initial INSERT (e.g. a trigger-assigned primary key). Dropping it is correct in the after_create context, but a brief comment explaining why it's safe to remove would help future readers.

✅ What's Well Done

• The slug_unique_violation? pattern is carefully designed. The false-positive prevention for canonical_slug, parent_slug, etc. is correct (\bslug\b respects word boundaries at . and ()) and thoroughly tested.
• The retry_if: ->(_error) { !persisted? } guard in retry_create_on_slug_unique_violation correctly prevents retrying an INSERT when the failure came from an after_create callback on an already-persisted record.
• The exhaustion fallback asymmetry (timestamp suffix for nullable slugs vs. raise for NOT NULL slugs) is intentional and well-documented.
• Test coverage is thorough, including the Rails state machine regression tests in around_create_state_machine_test.rb.
• SQL injection prevention in tests via conn.quote() is correct throughout.
• The PostgreSQL integration test is a nice addition and is properly opt-in.

🤖 Generated with Claude Code

PR Review: Fix race condition in slug generation with retry mechanism

Overall this is a well-structured, thoroughly documented solution to a real problem. The commit history shows a healthy iterative review process. Here is my analysis:

Correctness

with_slug_retry attempt counting is correct ✓

The counter starts at 0, increments after each failure, and exhausts at >= MAX_SLUG_GENERATION_ATTEMPTS. This gives exactly 10 total attempts: 1 initial + 9 retries before exhaustion (or raise). The test assertion MAX_SLUG_GENERATION_ATTEMPTS + 1 for the nullable path (10 attempts + 1 exhaustion call) is accurate.

slug_unique_violation? regex logic ✓

The pattern /\bslug\b|_on_slug\b/ correctly handles:

• SQLite: "UNIQUE constraint failed: table.slug" — period is a word boundary, so \bslug\b matches
• PostgreSQL: "Key (slug)=(value)" — parens are word boundaries, so \bslug\b matches
• MySQL index names: "index_posts_on_slug" — \bslug\b does NOT match (_ is a word character), but _on_slug\b does

The documented limitation for custom index names (e.g. uq_slug_col) is a safe false-negative: the exception bubbles up rather than causing silent data issues.

retry_if: ->(_error) { !persisted? } guard ✓

This correctly prevents retrying INSERT when a slug-named unique violation comes from an after_create callback (record already persisted at that point). The logic is sound.

Issues

🔴 Fragile source-reading test

test_exhaustion_fallback_raises_on_validation_error in race_condition_retry_test.rb reads the implementation source file and validates it with a regex:

source = File.read(file_path)
assert_match(/on_exhaustion.*\{.*self\.save!.*\}/m, source, ...)
assert_match(/with_slug_retry.*do.*self\.save!.*end/m, source, ...)

This tests implementation rather than behavior — it breaks silently if the source file is moved, reformatted, refactored, or if save! is extracted to a helper. The test comment itself acknowledges this tradeoff.

A behavioral test is preferable. The exhaustion path can be triggered by stubbing slug_unique_violation? to always return true on a model that also has a failing validation, then asserting ActiveRecord::RecordInvalid is raised directly.

🟡 Undocumented Rails behavior not covered by Appraisal

retry_create_on_slug_unique_violation calls yield multiple times via retry, relying on around_create yielding a re-invocable Proc. This is well-documented in code comments with Rails source references, and around_create_state_machine_test.rb validates the behavior. However, that test only runs against SQLite and is not in the standard Appraisal matrix.

A Rails upgrade could silently change this behavior. Suggestion: Add around_create_state_machine_test.rb to the Appraisal test run so it is exercised against all supported Rails versions in CI.

🟡 before_create re-execution is understated in CHANGELOG

The CHANGELOG says "before_create callbacks may fire multiple times." For any NOT NULL slug model that experiences a slug collision during create!, they will fire multiple times. Stronger wording would set clearer expectations.

🟡 Breaking change: save → save!

# Old: silent failure on validation error
self.save

# New: raises RecordInvalid on any failure
self.class.transaction(requires_new: true) { self.save! }

Documented in CHANGELOG and arguably a bug fix (silent failures are dangerous). However, any user who had additional validations that could fail during the post-create slug save now gets ActiveRecord::RecordInvalid where they previously got nothing. Worth emphasizing in release notes.

🟡 Asymmetric exhaustion has no escape hatch

• Nullable slug: exhaustion → timestamp fallback (lenient)
• NOT NULL slug: exhaustion → raises RecordNotUnique (strict)

Both are documented, but a user hitting sustained concurrency on popular slugs with a NOT NULL column gets an unhandled exception with no recovery path. Consider whether a configurable on_slug_exhaustion hook would be valuable, or document the recommended application-level mitigation.

Minor Observations

compute_base_slug ID-based branch — The else branch is documented as dead code (ID-based collisions are impossible). If it ever executed, it would produce a double-suffixed slug like d4735e3a265-1704067200-abcd1234. Since this is explicitly unreachable, raise NotImplementedError would be more defensive than silently returning an odd slug.

compute_slug_calls count assertion — assert_equal 2, race_model.compute_slug_calls in test_insert_time_slug_collision_retries_and_succeeds tests an implementation detail. A brief inline comment explaining the expected breakdown (1 from before_validation, 1 from pre_retry_action) would help future maintainers.

Test Coverage Assessment

The coverage is genuinely thorough:

• race_condition_retry_test.rb (351 lines): post-create slug retry
• insert_race_retry_test.rb (147 lines): pre-insert retry for NOT NULL slugs
• around_create_state_machine_test.rb (193 lines): Rails callback state machine validation
• slug_unique_violation_detection_test.rb (166 lines): exhaustive regex pattern coverage
• postgresql_insert_race_retry_test.rb (142 lines): optional PG integration

slug_unique_violation_detection_test.rb is exactly the right approach for testing a heuristic — it covers all three database error formats, false positives for columns like canonical_slug/parent_slug, and edge cases like nil messages and cause chains.

Summary

The implementation is solid. The main actionable items are: replace the source-reading test with a behavioral one, and ensure the around_create multi-yield test runs across all supported Rails versions in CI.

Code Review

Overall this is a well-thought-out solution to a real race condition. The problem statement is correct, the documentation is thorough, and the test coverage is extensive. Below is detailed feedback organized by severity.

Critical / High

1. Reliance on undocumented Rails behavior for around_create multi-yield

retry_create_on_slug_unique_violation calls retry inside a rescue block that wraps yield, causing yield to be invoked multiple times on the same around_create proc. The PR itself documents this prominently:

"This relies on Rails around_create yielding a re-invocable Proc, which is undocumented but has worked consistently in Rails 6-8."

This is the shakiest part of the implementation. If Rails ever changes the internal representation of callback chains, the retry silently fails (or worse, could re-invoke only part of the callback chain). The around_create_state_machine_test.rb suite mitigates this somewhat, but the risk of a silent breakage on a Rails upgrade remains.

Consider adding a note in the CI config identifying this test file as the canary for this behavior, and potentially filing an issue with Rails to ask for a supported re-invocable callback API.

2. Breaking change: save → save! in set_slug_with_retry

# Before
self.save

# After
self.class.transaction(requires_new: true) { self.save! }

This is documented in the CHANGELOG, but users who have models where validations can fail in the after-create slug-save phase will now get ActiveRecord::RecordInvalid raised instead of a silent no-op. The silent failure was arguably wrong, but this is a behavioral break for existing gem users. A rescue guard or a configurable option (raise_on_slug_save_failure) would be safer for a non-major version bump.

Medium

3. validates :slug, uniqueness: true creates a double-check race window

The retry loop in set_slug_with_retry catches ActiveRecord::RecordNotUnique from the DB, but save! first runs validates :slug, uniqueness: true (application-level SELECT). If another process steals the slug between the AR validation SELECT and the actual UPDATE, save! raises ActiveRecord::RecordInvalid — not RecordNotUnique. That exception is not caught by with_slug_retry.

In practice the DB constraint catches the real collision, but a comment noting this gap would be helpful. Alternatively, with_slug_retry could be extended to also rescue ActiveRecord::RecordInvalid and inspect the errors hash.

4. around_create adds overhead on every model create

included do
  around_create :retry_create_on_slug_unique_violation  # always registered
  ...
end

For nullable-slug models (the common case), retry_create_on_slug_unique_violation immediately does return yield unless slug_column_not_null? — correct, but a columns_hash lookup happens on every create. The schema cache makes it cheap, but a class-level memoized flag (set once after schema is loaded) would eliminate even that overhead.

5. slug_unique_violation? won't catch RecordInvalid from AR validation

Related to point 3: if you later extend with_slug_retry to handle RecordInvalid, the violation detection logic will need to inspect error.record.errors rather than exception messages, since RecordInvalid does not carry the DB-level error string.

Low / Suggestions

6. compute_base_slug has unreachable code for ID-based strategies

else
  # NOTE: This path should never execute in practice since ID-based slug
  # collisions are impossible (two records can't have the same ID).
  compute_slug
end

Unreachable code that produces a double-suffixed slug if somehow reached is a code smell. Consider replacing with raise NotImplementedError, "ID-based slug collisions should be impossible" or simply removing the branch with an explanatory comment.

7. compute_slug_for_retry is a transparent delegator but is private

def compute_slug_for_retry
  compute_slug
end

This exists as an extension point for subclasses to override. However, being private makes it harder to discover as a documented override point. Consider making it protected and documenting it explicitly as the intended customization hook.

8. with_slug_retry attempt semantics could be clearer

With MAX_SLUG_GENERATION_ATTEMPTS = 10, the block executes up to 10 times before exhaustion (1 initial + 9 retries), then the on_exhaustion proc adds 1 more attempt = 11 total saves in the worst case. The existing comment "1 initial + N-1 retries" is accurate but would benefit from a concrete example alongside the MAX_SLUG_GENERATION_ATTEMPTS constant.

9. SecureRandom.hex(4) upgrade is an improvement

The old SecureRandom.random_number(1000) gave only 1,000 possible fallback suffixes, making fallback collisions likely under any significant load. SecureRandom.hex(4) gives ~4 billion values. Good change.

10. == MAX_SLUG_GENERATION_ATTEMPTS → >= fix

Good defensive fix. The old == comparison would have been an infinite loop if attempts somehow skipped past the boundary (not possible with current code flow, but >= is strictly safer).

Test Coverage Notes

• around_create_state_machine_test.rb is an excellent addition — a Rails-version canary is exactly the right mitigation for the undocumented multi-yield behavior.
• postgresql_insert_race_retry_test.rb correctly guards on pg gem + env var and skips gracefully. Good pattern.
• slug_unique_violation_detection_test.rb would benefit from a test covering MySQL-format error messages (Duplicate entry 'val' for key 'index_posts_on_slug') and the case where the violation is in error.cause (PostgreSQL's PG::UniqueViolation chain).
• Consider a test documenting the known RecordInvalid gap (point 3) so future contributors understand the boundary.

Summary

The core logic is sound and addresses a real bug. The main concerns are: (a) the undocumented Rails multi-yield dependency, (b) the save → save! behavioral break for existing users, and (c) the RecordInvalid gap in the retry handler. The rest are refinements. The test suite is notably thorough. Happy to discuss any of these in more detail.

Code Review

This is solid work overall — the race condition is real, the solution is well-structured, and the test coverage is extensive. A few things worth discussing:

Undocumented Rails behavior in retry_create_on_slug_unique_violation

The comment itself flags this clearly, but it's the highest-risk part of the PR:

# NOTE: This calls `yield` multiple times (once per attempt) via `retry`.
# This relies on Rails `around_create` yielding a re-invocable Proc, which
# is undocumented but has worked consistently in Rails 6-8.

Calling yield multiple times inside around_create is not part of the public API. The around_create_state_machine_test.rb guard is a good mitigation, but this is still a load-bearing assumption about Rails internals. Consider adding a version check or a prominent runtime warning if this pattern should ever stop working. At minimum, the README should call out this caveat alongside the [!CAUTION] block already there.

Savepoint overhead on every create

For nullable-slug models (the default case), set_slug_with_retry now wraps every slug save in a savepoint:

self.class.transaction(requires_new: true) { self.save! }

The old code was self.save — no savepoint. This adds database round-trip overhead to every single record creation, even when there's zero chance of a race (e.g., a test environment, a single-writer setup). For high-throughput apps this could be measurable.

One option: only use requires_new: true when there's already an outer transaction open (self.class.connection.open_transactions > 0). That way single-writer creates stay cheap, and you only pay for the savepoint when it's actually necessary.

MAX_SLUG_GENERATION_ATTEMPTS semantics are off-by-one

The constant is named as a maximum number of attempts, but with_slug_retry actually makes MAX_SLUG_GENERATION_ATTEMPTS + 1 total attempts for the nullable-slug path (10 retried saves + 1 exhaustion fallback save). The test confirms this:

# MAX retries + 1 fallback attempt with timestamp suffix
assert_equal Slugifiable::Model::MAX_SLUG_GENERATION_ATTEMPTS + 1, race_model.update_attempts

Either rename the constant to MAX_SLUG_GENERATION_RETRIES, or adjust the semantics so exhaustion is one of the N attempts rather than an additional one. As-is, the name creates a reasonable expectation mismatch.

Source-inspection test is brittle

test_exhaustion_fallback_raises_on_validation_error reads the gem's source file and pattern-matches it:

source_location = model.method(:set_slug_with_retry).source_location
file_path, _line = source_location
source = File.read(file_path)
assert_match(/on_exhaustion.*\{.*self\.save!.*\}/m, source, ...)

This will break on any reformatting and doesn't test actual behavior. Better to test behaviorally: create a model where a custom validation always fails after MAX attempts and assert RecordInvalid is raised. The test comment acknowledges it's a simplification, but it would be worth the behavioral test instead.

slug_unique_violation? false negatives with non-standard index names

The LIMITATION comment documents this well:

Custom index names like uq_slug_col, slug_idx, or posts_slug_unique won't match.

Worth calling out in the README or a "Limitations" section, since users who've customized their index names will find the retry silently not working — the exception just bubbles up as if this PR never happened. That's safe (no data corruption) but potentially confusing.

Minor: retry_if lambda ignores its argument

with_slug_retry(
  -> { self.slug = compute_slug_for_retry },
  retry_if: ->(_error) { !persisted? }
) do

The with_slug_retry signature accepts retry_if: ->(_error) { true } but the callable is passed the error object and then ignores it with _error. Since retry_if in retry_create_on_slug_unique_violation only checks !persisted?, the error argument is unused everywhere. Either drop it from the signature or use it consistently.

What's working well

• The two-path design (nullable vs NOT NULL) is clean and correctly reasoned.
• slug_unique_violation? handles SQLite, PostgreSQL, and MySQL error formats with good false-positive prevention for columns like canonical_slug.
• Extensive test suite, including the Rails state machine regression tests.
• Breaking changes are clearly documented in the CHANGELOG.
• compute_slug_for_retry as an override point is a thoughtful extension hook.
• The retry_if: ->(_error) { !persisted? } guard preventing double-retry on already-persisted records is a subtle but important correctness detail.

PR Review: Fix race condition in slug generation with retry mechanism

Overall this is a well-thought-out fix for a real problem, with thorough documentation and comprehensive tests. Here are my findings:

Correctness / Logic Issues

1. compute_base_slug has an inconsistent fallback for :attribute_missing

In compute_slug_based_on_attribute, a missing attribute falls back to compute_slug_as_string (hex string, e.g. "d4735e3a265"). But in compute_base_slug, the same condition falls back to generate_random_number_based_on_id_hex (integer, e.g. 321678). This means the exhaustion fallback can produce a slug like 321678-1704067200-abcd1234 where normal slug generation would have produced d4735e3a265 — a different type and format.

The comment acknowledges this is intentional ("All fallback cases use numeric ID-based slug for exhaustion path"), but the inconsistency is subtle. If both must differ, it's worth a stronger comment explaining why the exhaustion path intentionally diverges from the normal fallback.

2. after_find path now uses save\! without documentation

update_slug_if_nil calls set_slug, which now calls set_slug_with_retry, which uses save\!. This means nil-slug repair triggered on after_find now raises ActiveRecord::RecordInvalid on validation failure, instead of silently skipping. The CHANGELOG documents the save\! change for the after-create path but doesn't call out that after_find → update_slug_if_nil is equally affected. For any app that has nil slugs and custom validations, this is a user-facing behavioral change on record load.

Reliability / Risk

3. Undocumented Rails behavior: multi-yield from around_create

retry_create_on_slug_unique_violation calls yield inside a retry block, relying on Rails' around_create yielding a re-invocable Proc. The comments acknowledge this is undocumented (model.rb:330). The around_create_state_machine_test.rb is a good safeguard, but this is still the highest-risk part of the implementation. If Rails internalizes state across the yield boundary in a future version, this could produce corrupted records silently.

Consider documenting which exact Rails versions this has been tested against in the comments (rather than just "Rails 6-8").

4. Savepoint overhead on every NOT NULL slug model create

Every create on a model with a NOT NULL slug column now runs inside a savepoint (requires_new: true), even when there is no collision. This is correct for PostgreSQL safety, but adds database round-trips on the happy path. For applications doing bulk inserts of NOT NULL-slug models, this could be measurable. Worth a note in the README or a configurable opt-out if performance becomes a concern.

Test Quality

5. test_exhaustion_fallback_raises_on_validation_error inspects source code

race_condition_retry_test.rb:1244-1255 uses File.read + regex matching on the implementation file to assert save\! is used. This is fragile — a whitespace change, method extraction, or lambda-to-proc refactor will break the test without any behavioral regression. A behavioral test (even an indirect one) would be more durable. The test even acknowledges this limitation in a comment.

6. compute_slug_calls == 2 assertion depends on implementation detail

insert_race_retry_test.rb:660-661 asserts exactly 2 compute_slug calls: once in before_validation and once in compute_slug_for_retry. This is only true because ensure_slug_for_insert guards with if slug.blank?. If a subclass omits that guard (or uses a different pattern), the count changes. The assertion is correct for the test setup, but fragile for generalization. A comment explaining the exact call sequence would help future readers.

7. No direct test that nullable-slug models skip the savepoint

The optimization return yield unless slug_column_not_null? (model.rb:344) is correct and efficient, but there is no test asserting that nullable-slug models do NOT use savepoints. This makes it harder to catch a regression where the guard is accidentally removed.

Minor / Nits

8. AR uniqueness validation is now a third collision-prevention layer

With the DB-level unique index now added to the test schema (test_helper.rb:1449), the collision chain is: generate_unique_slug (SELECT loop) + validates :slug, uniqueness: true (SELECT) + DB constraint (INSERT). This is correct behavior — just worth being aware of when reasoning about the number of queries per create.

9. compute_base_slug else branch is dead code with a footgun

model.rb:147 calls compute_slug in the else branch and explicitly documents it produces a double-suffixed slug if ever reached. Since it is acknowledged dead code, it could raise NotImplementedError or simply be removed to make the invariant explicit, rather than silently producing a malformed slug in an impossible-but-not-enforced code path.

10. Regex comment is slightly misleading

model.rb:319 says \bslug\b matches the SQLite format "table.slug" because the period is a word boundary — that is correct. But the comment then lists two separate match cases (\bslug\b and _on_slug\b) without clearly explaining which pattern covers which database format. Minor readability nit.

Summary

The most actionable items before merge: (2) document the after_find behavioral change in the CHANGELOG, and (5) consider replacing the source-inspection test with a behavioral test if feasible.

🤖 Generated with Claude Code

test comment please ignore - will delete

PR Review: Race condition retry mechanism

This PR addresses a real problem and the overall approach is well thought-out. The documentation (CHANGELOG, README, inline comments) is thorough. Here are my findings:

Critical Issues

1. Reliance on undocumented Rails behavior (around_create multi-yield)

retry_create_on_slug_unique_violation calls yield multiple times via Ruby retry. The PR comment already acknowledges this is undocumented:

This relies on Rails around_create yielding a re-invocable Proc, which is undocumented but has worked consistently in Rails 6-8.

This is the most significant concern in the PR. Calling yield multiple times in an around_create callback is not part of Rails public contract. The around_create_state_machine_test.rb is a good safeguard, but it only catches the breakage after a Rails upgrade ships, not before.

A safer alternative for NOT NULL columns would be to abandon the around_create multi-yield approach and instead make ensure_slug_for_insert (or an equivalent before_validation hook) collision-aware: e.g., by trying a different random suffix on each invocation rather than relying on an outer retry loop that re-runs the entire callback chain.

2. save to save! is a breaking change with runtime impact

The CHANGELOG documents this but it is worth calling out: replacing self.save with self.save! means any app that had a validation failure during the after-create slug-save phase will now raise ActiveRecord::RecordInvalid instead of silently continuing. This could manifest as a 500 in production on a code path that previously worked (by silently dropping the slug).

3. RecordInvalid from a race condition is not retried

with_slug_retry only rescues ActiveRecord::RecordNotUnique. But when the race window is the AR uniqueness validator SELECT to UPDATE gap, the exception will be RecordInvalid (validation fails, not the DB constraint). The PR notes this as a known limitation:

If another process steals the slug between the AR uniqueness validation SELECT and the actual UPDATE, save! raises RecordInvalid instead of RecordNotUnique. This is a narrow window...

The window is narrow but it is exactly the race described in the PR summary. The DB constraint is the ultimate guard, but that raises RecordNotUnique — the case the retry loop would catch. For the validation-layer race, the fix does nothing. To be complete, with_slug_retry should also rescue ActiveRecord::RecordInvalid when the error is slug-uniqueness-related.

Medium Issues

4. before_create callbacks fire multiple times — behavioral change is underemphasized

The CHANGELOG notes this as Breaking Change (Behavioral) but any app using before_create for emails, webhooks, or job enqueueing will silently start doing these twice on any slug collision. This is a non-obvious footgun that could cause duplicate emails in production. Consider whether this warrants a more prominent warning, e.g., a separate section in the README or a runtime warn when around_create actually retries.

5. compute_base_slug is a parallel implementation that can diverge

compute_base_slug re-implements slug strategy resolution independently of compute_slug_based_on_attribute. The two must stay in sync or the exhaustion fallback produces different slug formats than the normal path. The else branch (for ID-based strategies) explicitly calls compute_slug and notes it could double-suffix — that is a sign this branch should not exist. If ID-based slug collisions are impossible (as stated), the else branch is dead code. Dead code in a retry path is hard to reason about.

Minor Issues

6. around_create callback runs for all models, including nullable-slug ones

Every create on any model including Slugifiable::Model now invokes retry_create_on_slug_unique_violation, which calls slug_persisted? and slug_column_not_null? before bailing out. The columns_hash call is memoized by Rails so the overhead is small, but it is still a callback invocation on every single create, for the common case (nullable slugs, no collision). Registering the callback only when null: false is detected at include-time would eliminate this overhead.

7. Test test_exhaustion_fallback_raises_on_validation_error reads source files

Testing implementation by parsing source code is fragile — it will silently pass even if the regex stops matching due to whitespace changes, and it tests the wrong thing (code structure, not behavior). The behavioral test test_exhaustion_fallback_raises_on_continued_unique_violation is the right approach; this source-parsing test should be removed.

8. slug_unique_violation? documented as incomplete for custom index names

The regex works well for SQLite, PostgreSQL, and standard Rails index naming. The explicit documentation of the false-negative limitation is appreciated. The safe failure mode (exception bubbles up, no retry) is the correct outcome.

Summary

The core race condition being fixed is real and the solution is mostly sound. The main things to address before merge:

1. The around_create multi-yield approach needs either a stronger justification or an alternative design — undocumented Rails behavior is the fragility most likely to cause a silent breakage after a Rails upgrade.
2. RecordInvalid from the uniqueness validation race should be retried — or explicitly documented as this PR does not fix the validation-layer race, only the DB-constraint-layer race.
3. Remove the source-parsing test — replace with a proper behavioral test.

The test coverage is extensive and the documentation is detailed. The explicit acknowledgment of limitations (undocumented Rails behavior, known RecordInvalid gap, MySQL index names) shows good awareness of edge cases.

deleting test

CLAUDE.md

This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository.

Please go ahead and read the full context for this project at .cursor/rules/0-overview.mdc and .cursor/rules/1-quality.mdc now. Also read the README for a good overview of the project.

PR Review: Race condition retry mechanism

This PR addresses a real problem and the overall approach is well thought-out. The documentation (CHANGELOG, README, inline comments) is thorough. Here are my findings:

Critical Issues

1. Reliance on undocumented Rails behavior (around_create multi-yield)

retry_create_on_slug_unique_violation calls yield multiple times via Ruby retry. The PR comment already acknowledges this is undocumented:

"This relies on Rails around_create yielding a re-invocable Proc, which is undocumented but has worked consistently in Rails 6-8."

This is the most significant concern in the PR. Calling yield multiple times in an around_create callback is not part of Rails' public contract. The around_create_state_machine_test.rb is a good safeguard, but it only catches the breakage after a Rails upgrade ships, not before.

A safer alternative for NOT NULL columns would be to abandon the around_create multi-yield approach and instead make ensure_slug_for_insert (or an equivalent before_validation hook) collision-aware — e.g., by trying a different random suffix on each invocation rather than relying on an outer retry loop that re-runs the entire callback chain.

2. save → save! is a breaking change with runtime impact

The CHANGELOG documents this but it's worth calling out: replacing self.save with self.save! means any app that had a validation failure during the after-create slug-save phase will now raise ActiveRecord::RecordInvalid instead of silently continuing. This could manifest as a 500 in production on a code path that previously "worked" (by silently dropping the slug).

3. RecordInvalid from a race condition is not retried

with_slug_retry only rescues ActiveRecord::RecordNotUnique. But when the race window is the AR uniqueness validator SELECT → UPDATE gap, the exception will be RecordInvalid (validation fails, not the DB constraint). The PR notes this as a "known limitation":

"If another process steals the slug between the AR uniqueness validation SELECT and the actual UPDATE, save! raises RecordInvalid instead of RecordNotUnique. This is a narrow window..."

The window is narrow but it's exactly the race described in the PR summary. The DB constraint is the ultimate guard, but that raises RecordNotUnique — the case the retry loop would catch. For the validation-layer race, the fix does nothing. To be complete, with_slug_retry should also rescue ActiveRecord::RecordInvalid when the error is slug-uniqueness-related.

Medium Issues

4. before_create callbacks fire multiple times — behavioral change is underemphasized

The CHANGELOG notes this as "Breaking Change (Behavioral)" but any app using before_create for emails, webhooks, or job enqueueing will silently start doing these twice on any slug collision. This is a non-obvious footgun that could cause duplicate emails in production. Consider whether this warrants a more prominent warning, e.g., a separate section in the README or a runtime warn when around_create actually retries.

5. compute_base_slug is a parallel implementation that can diverge

compute_base_slug re-implements slug strategy resolution independently of compute_slug_based_on_attribute. The two must stay in sync or the exhaustion fallback produces different slug formats than the normal path. The else branch (for ID-based strategies) explicitly calls compute_slug and notes it could double-suffix — that's a sign this branch should not exist. If ID-based slug collisions are impossible (as stated), the else branch is dead code. Dead code in a retry path is hard to reason about.

Minor Issues

6. around_create callback runs for all models, including nullable-slug ones

Every create on any model including Slugifiable::Model now invokes retry_create_on_slug_unique_violation, which calls slug_persisted? and slug_column_not_null? before bailing out. The columns_hash call is memoized by Rails so the overhead is small, but it's still a callback invocation on every single create, for the common case (nullable slugs, no collision). Registering the callback only when null: false is detected at include-time would eliminate this overhead.

7. Test test_exhaustion_fallback_raises_on_validation_error reads source files

source = File.read(file_path)
assert_match(/on_exhaustion.*\{.*self\.save!.*\}/m, source, ...)

Testing implementation by parsing source code is fragile — it will silently pass even if the regex stops matching due to whitespace changes, and it tests the wrong thing (code structure, not behavior). The behavioral test test_exhaustion_fallback_raises_on_continued_unique_violation is the right approach; this source-parsing test should be removed.

8. slug_unique_violation? documented as incomplete for custom index names

The regex works well for SQLite, PostgreSQL, and standard Rails index naming. The explicit documentation of the false-negative limitation is appreciated. The safe failure mode (exception bubbles up, no retry) is the correct outcome.

Summary

The core race condition being fixed is real and the solution is mostly sound. The main things to address before merge:

1. The around_create multi-yield approach needs either a stronger justification or an alternative design — undocumented Rails behavior is the fragility most likely to cause a silent breakage after a Rails upgrade.
2. RecordInvalid from the uniqueness validation race should be retried — or explicitly documented as "this PR does not fix the validation-layer race, only the DB-constraint-layer race."
3. Remove the source-parsing test — replace with a proper behavioral test.

The test coverage is extensive and the documentation is detailed. The explicit acknowledgment of limitations (undocumented Rails behavior, known RecordInvalid gap, MySQL index names) shows good awareness of edge cases.

test comment from gh cli

Code Review

This is a well-motivated fix with thorough documentation and test coverage. The overall approach is sound. Below are my findings, ranging from significant concerns to minor observations.

Significant Issues

1. save\! in update_slug_if_nil is a high-risk breaking change

set_slug_with_retry (now called by both after_create and update_slug_if_nil) uses save\! instead of the old save. This is documented in the CHANGELOG, but the severity deserves more attention.

The after_create path is largely fine — failures there are loud and expected. The update_slug_if_nil path is the problem: it runs on every after_find for records with a nil slug. This means loading a record in production after upgrading could suddenly raise ActiveRecord::RecordInvalid if the model has any failing validation (even one unrelated to slugs). Users with large existing datasets of nil slugs, or models with conditional validations, are silently at risk.

Consider splitting these paths: use save\! for the initial after_create path but use save (with logging on failure) for the update_slug_if_nil repair path. Alternatively, rescue RecordInvalid in update_slug_if_nil and log a warning without re-raising.

2. around_create multi-yield relies on undocumented Rails internals

retry_create_on_slug_unique_violation calls retry inside a rescued block, which re-invokes yield on the around_create block multiple times. The PR acknowledges this (with a detailed comment and around_create_state_machine_test.rb), but the framing undersells the risk.

This behavior is not guaranteed by Rails' public API. A minor Rails internal refactor could break it silently — the regression test would catch it, but only if it runs on every Rails upgrade. This test needs to be part of the CI matrix for all supported Rails versions.

An alternative that avoids this entirely: catch the collision in after_create_commit (or after_rollback + re-save). More complex, but does not depend on undocumented behavior. At minimum, add an explicit note in AGENTS.md about this dependency.

3. RecordInvalid gap is underdocumented

The with_slug_retry comment notes:

If another process steals the slug between the AR uniqueness validation SELECT and the actual UPDATE, save\! raises RecordInvalid instead of RecordNotUnique. This is a narrow window... but be aware that RecordInvalid from a race is not retried.

The consequence is concrete: after all the retry machinery, there is still a race window where the retry does nothing. Users reading the PR description ("handles race conditions") may not realize this gap exists. The README should mention it alongside the new caution blocks.

Moderate Issues

4. test_exhaustion_fallback_raises_on_validation_error reads source code (fragile test)

source_location = model.method(:set_slug_with_retry).source_location
file_path, _line = source_location
source = File.read(file_path)
assert_match(/on_exhaustion.*\{.*self\.save\!.*\}/m, source, ...)

This test will fail if the method is renamed, the file is reorganized, or code is extracted to a module. It tests the implementation rather than the behavior. If triggering the actual behavior with an anonymous class has Rails naming issues, the correct approach is to define a named class in a test fixture. Testing source text is not a reliable substitute.

5. Inconsistent suffix format between generate_unique_slug and the exhaustion fallback

generate_unique_slug appends a decimal number suffix (e.g. my-slug-123456), while the exhaustion fallback produces my-slug-1716144000-ab12cd34. These are cosmetically inconsistent and make it harder for users to reason about slug formats. Since the PR already changed the exhaustion path to SecureRandom.hex(4), consider standardizing on hex throughout.

6. Misleading comment in test_max_retries_exceeded_raises_error

# MAX retries + 1 fallback attempt with timestamp suffix
assert_equal Slugifiable::Model::MAX_SLUG_GENERATION_ATTEMPTS + 1, race_model.update_attempts

The assertion is correct but the comment does not make clear that the +1 comes from the exhaustion fallback also issuing a save\!. Clearer: # 10 retry attempts + 1 exhaustion fallback attempt = 11 total UPDATE calls.

Minor / Nits

7. slug_unique_violation? override point is not discoverable

The comment correctly documents the custom-index-name limitation. Worth adding a README note that users with custom index names can override slug_unique_violation?, following the same SUBCLASS OVERRIDE POINT pattern used for compute_slug_for_retry.

8. Unreachable else branch in compute_base_slug could be more defensive

else
  # NOTE: This path should never execute in practice...
  compute_slug
end

"Should never execute" is the kind of invariant that benefits from a defensive warning log. If triggered unexpectedly, a double-suffix slug like d4735e3a265-1716144000-abcd1234 would be silently written with no indication something went wrong.

What's done well

• The slug_unique_violation? regex (/\bslug\b|_on_slug\b/) correctly avoids false positives on columns like canonical_slug, parent_slug, user_slug. The test coverage across SQLite, PostgreSQL, and MySQL error formats is thorough and well-structured.
• The retry_if: ->(_error) { \!persisted? } guard in retry_create_on_slug_unique_violation correctly handles the edge case where a collision comes from an after_create callback on an already-persisted record.
• Savepoints (requires_new: true) around each attempt correctly keep PostgreSQL outer transactions usable after a rollback.
• around_create_state_machine_test.rb is good proactive insurance against Rails upgrade regressions.
• compute_slug_for_retry being protected with an explicit override point is a nice extension pattern.
• The PostgreSQL integration test is correctly optional and well-structured.

Summary

The core approach is sound and this clearly improves the gem's behavior under concurrency. Main areas to address before merge:

1. Reconsider save\! in the update_slug_if_nil path — or rescue RecordInvalid there to avoid surprise exceptions on record loads after upgrading
2. Ensure the around_create multi-yield regression test runs in CI across all supported Rails versions
3. Replace the source-reading test with a behavioral test
4. Add a README note about the RecordInvalid race gap that the retry does not cover

PR Review: Fix race condition in slug generation with retry mechanism

This PR tackles a real concurrency problem with a well-structured solution. The documentation is thorough, the test coverage is comprehensive, and the design has clearly gone through multiple refinements. Here's detailed feedback:

Summary

The approach is sound: wrapping slug saves in savepoints and retrying on RecordNotUnique is the correct way to handle optimistic concurrency conflicts. The two-path architecture (UPDATE path via set_slug_with_retry, INSERT path via retry_create_on_slug_unique_violation) correctly separates nullable vs NOT NULL slug schemas.

Issues & Concerns

🔴 Critical: save! in after_find path can break record loading

The save → save! change propagates through update_slug_if_nil → set_slug → set_slug_with_retry. This means any call to Model.find(id) on a record with a nil slug that fails validation during repair will now raise an exception when loading data.

# This pattern is now potentially dangerous for legacy data with nil slugs:
user = User.find(id)  # Could raise ActiveRecord::RecordInvalid

For the after_create path, raising is arguably correct behavior. For after_find, it's highly disruptive — applications generally don't expect a SELECT to raise. If a production DB has records with nil slugs (from before the gem was added, migration issues, direct SQL inserts, etc.), the first read of those records will now throw.

Consider keeping save in the update_slug_if_nil path (rescue and log instead), or at minimum adding a rescue guard in update_slug_if_nil itself.

🟠 Significant: Reliance on undocumented Rails behavior

retry_create_on_slug_unique_violation calls yield multiple times via retry, relying on around_create wrapping a re-invocable Proc. This is noted in the code comments and there's a regression test (around_create_state_machine_test.rb) to catch breakage, which is good.

However, the practical implication should be highlighted more prominently in the README beyond the current CAUTION block. The before_create multiple-execution warning applies to any model using null: false on the slug column, even when the common case (no collision) is unaffected.

The comment says "has worked consistently in Rails 6-8" — it would be worth noting what specific Rails version the test suite validated against, so future maintainers know when the test was last verified.

🟠 Significant: Savepoint overhead on every create for NOT NULL slug models

retry_create_on_slug_unique_violation wraps every create in a savepoint for models where slug_column_not_null? is true, regardless of whether a collision actually occurs. For high-write applications this is measurable overhead (savepoints require round-trips in PostgreSQL).

This is a necessary cost for correctness, but it's an implicit behavioral change for all users who adopted null: false slug columns following the old README guidance (which previously discouraged it). A note in the README about this trade-off would help.

🟡 Minor: validates :slug, uniqueness: true re-executes on every INSERT retry

Since yield re-runs all callbacks, each INSERT retry re-runs before_validation, which triggers the uniqueness validation — an additional SELECT per retry attempt on top of the savepoint overhead. For 10 retries that's up to 10 extra SELECTs + 10 savepoints. This is probably fine in practice (slug collisions should be rare), but worth documenting.

🟡 Minor: Asymmetric exhaustion behavior is potentially surprising

• UPDATE path: Exhausts → timestamp fallback (record still saves)
• INSERT path: Exhausts → raises RecordNotUnique (create fails)

This is documented in CHANGELOG and README, but the user experience is notably different depending on their schema choice. The README could benefit from a comparison table or clearer "choose your schema" guidance that surfaces these differences upfront, rather than in separate callout boxes.

🟡 Minor: test_max_retries_exceeded_raises_error expectation fragility

# MAX retries + 1 fallback attempt with timestamp suffix
assert_equal Slugifiable::Model::MAX_SLUG_GENERATION_ATTEMPTS + 1, race_model.update_attempts

This asserts that before_update fires once more during the on_exhaustion save. This is an implementation detail — if on_exhaustion were changed to bypass callbacks (e.g., use update_column), this count would change. The test verifies incidental behavior rather than the contract. Consider asserting the outcome instead: that RecordNotUnique was raised after all retry attempts failed.

🟡 Minor: compute_base_slug else branch is dead code with a landmine comment

else
  # NOTE: This path should never execute in practice...
  # If it ever did, the exhaustion fallback would produce a double-suffixed slug
  compute_slug
end

The comment correctly identifies the issue (double-suffixing). If this path truly can't happen, consider raising NotImplementedError instead, which makes the failure mode explicit. Leaving compute_slug here silently produces a wrong result, which is worse than a clear error.

Positive Notes

• The with_slug_retry abstraction cleanly eliminates duplication between the two retry paths
• slug_unique_violation?'s regex (/\bslug\b|_on_slug\b/) is well-considered and the false-positive analysis in the comments is accurate
• SecureRandom.hex(4) in the timestamp fallback is a good improvement over random_number(1000) — 32-bit entropy vs ~10-bit
• The compute_slug_for_retry protected override point is a nice extensibility hook
• slug_column_not_null? correctly uses AR's schema cache rather than duplicating memoization
• The around_create_state_machine_test.rb will catch Rails-upgrade regressions
• The retry_if: ->(_error) { !persisted? } guard correctly prevents the INSERT retry from firing when the RecordNotUnique comes from after_create cleanup (post-persist)

Suggested Changes (priority order)

1. Guard update_slug_if_nil's slug repair against propagating exceptions during after_find — or document clearly that nil-slug records will cause exceptions on first load
2. Add a version bump (this has breaking changes)
3. Clarify the dead-code compute_base_slug else branch — raise instead of silently double-suffixing
4. Strengthen the test_max_retries_exceeded_raises_error test to assert outcome rather than call count

Overall this is a solid fix for a real problem. The breaking change in after_find behavior is the main thing I'd want addressed before merging.